3 ms·

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_varia

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.

Cthulhu_ 3y ago
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.
- Ensorceled 3y ago
  It was found during code review AND it revealed a gap in our unit tests. So, 50% success, I guess. Belt and suspenders.
throw1234651234 3y ago
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.
- Ensorceled 3y ago
  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.
  - throw1234651234 3y ago
    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.
Izkata 3y ago
> 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.

Cthulhu_ 3y ago

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.

Ensorceled 3y ago
It was found during code review AND it revealed a gap in our unit tests. So, 50% success, I guess. Belt and suspenders.

throw1234651234 3y ago

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.

Ensorceled 3y ago
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.
- throw1234651234 3y ago
  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.

Izkata 3y ago

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