• (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)

    In my experience, things like admin panels are extremely volatile. Project Management will often ask for multiple changes within the same sprint to tweak or adjust how things are rendered or for whom - usually with contradictory requirements. You can quickly get into a place several sprints in where nobody is really sure who's supposed to have access to which toggles under what conditions. :/

  • 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.

  • Idk some nerd (unregistered) in reply to Hmmmm

    It seems fairly common and idiomatic (in my limited experience).

  • (nodebb)

    My guess would be that there used to be more controls. The heading would be displayed if any of the controls were available - something like "(isAdmin || canSeeResults || canSeeSummary || canSeeDetails). Then each control would have (isAdmin || canSeeSummary) etc. So different users would see different combinations of controls, but the heading would always appear, unless the user could see none of the controls. Then all but one of the controls was deleted, making the conditions identical and the inner check redundant.

Leave a comment on “Check and Check”

Log In or post as a guest

Replying to comment #700133:

Log Out

« Return to Article