• (nodebb)

    My least-worst guess is that it was originally just the header with the conditional button, and then someone said, "That empty header without the button looks pretty stupid. You should hide it," so the programmer wrapped the header in the same condition, on the "smallest change possible" theory.

  • some guy (unregistered)

    Though in this case, I don't think it adds any safety.

    It did add breakage to the syntax highlighting, so that's something.

  • 516052 (unregistered)

    My guess is that at some point someone was copy pasting things and did not bother to think too much.

  • Brian (unregistered)

    This isn't necessarily wrong - the real question is what is the backend doing? If you've got proper security in your API, then the client-side conditions are merely to avoid rendering empty sections and non-functional controls. Considering what site we're on, though, I'd wager that's not the case here.

  • Brian (unregistered) in reply to Brian

    Oops, disregard that previous comment. My brain apparently filtered out that second conditional - I read it as a "dumb client-side security" post, not a "dumb unnecessary code" post.

  • Hmmmm (unregistered)

    Haven't coded with it, but it seems odd (read, "mangling") to mix boolean operators with rendering blocks.

  • Lothar (unregistered)

    Those admin rights can be revoked fast, so better check regularily if they're still there.

  • LegacyWarrior (unregistered)
    Comment held for moderation.
  • LegacyWarrior (unregistered) in reply to Steve_The_Cynic

    I'd actually put my money on the isAdmin || canSeeResults check originally being two separate sections to render "admin results" and "non-admin results" that were later combined and the dev didn't think to check what is likely several hundred lines above for how to simplify the checks. And who knows, PM will likely change their mind tomorrow anyway.

Leave a comment on “Check and Check”

Log In or post as a guest

Replying to comment #700127:

Log Out

« Return to Article