• steventhedev@lemmy.world
    link
    fedilink
    arrow-up
    68
    arrow-down
    1
    ·
    2 months ago

    Ew no.

    Abusing language features like this (boolean expression short circuit) just makes it harder for other people to come and maintain your code.

    The function does have opportunity for improvement by checking one thing at a time. This flattens the ifs and changes them into proper sentry clauses. It also opens the door to encapsulating their logic and refactoring this function into a proper validator that can return all the reasons a user is invalid.

    Good code is not “elegant” code. It’s code that is simple and unsurprising and can be easily understood by a hungover fresh graduate new hire.

    • traches@sh.itjust.works
      link
      fedilink
      English
      arrow-up
      48
      arrow-down
      1
      ·
      2 months ago

      Agreed. OP was doing well until they replaced the if statements with ‚function call || throw error’. That’s still an if statement, but obfuscated.

      • BrianTheeBiscuiteer@lemmy.world
        link
        fedilink
        arrow-up
        7
        ·
        2 months ago

        Don’t mind the || but I do agree if you’re validating an input you’d best find all issues at once instead of “first rule wins”.

        • rooster_butt@lemm.ee
          link
          fedilink
          arrow-up
          3
          ·
          2 months ago

          Short circuiting conditions is important. Mainly for things such as:

          if(Object != Null && Object.HasThing) …

          Without short circuit evaluation you end up with a null pointer exception.

    • verstra
      link
      fedilink
      arrow-up
      20
      ·
      2 months ago

      I agree, this is an anti-pattern for me.

      Having explicit throw keywords is much more readable compared to hiding flow-control into helper functions.

    • YaBoyMax
      link
      fedilink
      English
      arrow-up
      15
      arrow-down
      1
      ·
      2 months ago

      This is the most important thing I’ve learned since the start of my career. All those “clever” tricks literally just serve to make the author feel clever at the expense of clarity and long-term manintainability.

    • hex
      link
      fedilink
      arrow-up
      6
      ·
      2 months ago

      I mean, boolean short circuit is a super idiomatic pattern in Javascript

      • arendjr
        link
        fedilink
        arrow-up
        7
        ·
        2 months ago

        I think that’s very team/project dependent. I’ve seen it done before indeed, but I’ve never been on a team where it was considered idiomatic.

        • hex
          link
          fedilink
          arrow-up
          2
          ·
          2 months ago

          It’s not that deep. It looks nice, and is easy to understand.

    • Womble@lemmy.world
      link
      fedilink
      English
      arrow-up
      10
      arrow-down
      5
      ·
      edit-2
      2 months ago

      Good code is not “elegant” code. It’s code that is simple and unsurprising and can be easily understood by a hungover fresh graduate new hire.

      I wouldnt go that far, both elegance are simplicity are important. Sure using obvious and well known language feaures is a plus, but give me three lines that solve the problem as a graph search over 200 lines of object oriented boilerplate any day. Like most things it’s a trade-off, going too far in either direction is bad.

    • sip
      link
      fedilink
      arrow-up
      1
      ·
      edit-2
      2 months ago

      assert(isPasswordGood(…)) is already in the language. node