Hacker News new | past | comments | ask | show | jobs | submit login

I have a hard time getting developers to follow this type of advice and a lot of developers will "clean up" and "optimize" the example code in introducing_variable_version by eliminating the paid_today variable back into a single if statement.

I recently had a developer "improve" a security sensitive series of checks (think a series of if statements: if logged in, if not inactive, if in required group, etc.) by merging the statements into a single if statement that, unsurprisingly, broke the check and left a security hole.




Yeah that's a constant struggle, developers thinking less and / or clever code is better code; it's self-gratification, and it should be stopped during code review. To them I'd say I'm sorry a series of simple if- statements doesn't challenge you intellectually, but the correctness of these checks are more important than your ego.

Of course, there's also a gap in unit tests if they can change an important check without a test failing.


It was found during code review AND it revealed a gap in our unit tests.

So, 50% success, I guess.

Belt and suspenders.


What was bad about it? Did they just do:

bool hasAccess = HasAccess(token);

private bool HasAccess = token: Jwt => { return IsLoggedIn(token) && IsActive(token) && HasRbacPermission(token); }

If so, what's wrong with that? It moves the complexity of HasAccess out of whatever is checking for access, before presumably doing other things.

* I have each check on its own line, don't want to reformat message to take up space here.


It was fairly long series of checks, some complicated, and they mixed up an or and not case when merging them into the single if.


Ah, I see now. I think it's a very common mistake. People also mess up negation when they refactor things like these. So your original was

if(!loggedIn) return Unauthorized;

if(isTokenExpired) return Unauthorized;

etc? I think being that explicit about it works pretty well sometimes.


> I have a hard time getting developers to follow this type of advice

Check out some of the other comments on here: It's bad advice because they don't account for a whole bunch of other things.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: