109

Peer review is a life saver!!! My colleague just saved me my job as i almost published this fucking block to production.

Comments
  • 17
    Happens to the best of us. Great job on getting it caught!
  • 16
    Kudos to the team finding this and on you recognizing the mistake. This was a very important lesson my friend! thanks for sharing with us!
  • 43
    and this people, is why you having staging environments.

    Manual testing and even unit testing would pick this up before you got that to production.
  • 20
    Uhh...no testing?
  • 8
    Negated if's are always confusing :3
  • 1
    Burned by C#'s rudimentary type system I guess… and while I realize that this is an existing codebase, and can probably not be refactored to use a stronger domain model, I dare inject here that using a stronger typed language such as F# would have allowed to produce a domain model where this mistake would have been quite impossible to make. Google 'Domain Modeling Made Functional' if you are interested.
  • 0
    Why would you publish this in production if it wasn't her/him? Why wouldn't you *test* such job-threatening system-critical code?
  • 2
    This is why we prefer x == false instead of !x

    We’ve all been bitten by this bug.
  • 3
    @hatemyjob Why would using ``x == false`` instead of ``!x`` make any difference at all?
  • 6
    @SomeNone logically, no difference.
    Legibility though, it's harder to miss read == / === over !x
  • 1
    This should have been tested through unit tests lol

    These things can be missed with peer reviews as well :(
  • 3
    @SomeNone slight bump in readability.
    “==false” has less chances of being mistaken with “x”

    than

    “!x” instead of “x”

    Functionally the same. The likelihood of skimming and missing this bug is far far less if you use the method no 1.
  • 4
    For reasons like this I prefer to write:

    if( a == false)

    If OP don't mind me asking, why ResponseStatusCode is inside Constants class and not an enum on its own? like HttpStatusCode?
  • 0
    @gitpush ResponseStatusCode is an enum inside Constants class which holds other types of status code and it's also socket related not http. Would find other ways of improving it.
  • 3
    How could a critical bug like that make it's way past your own QA as you're developing? Did you just write the code, hit compile and pour yourself a beer?? You guys don't have any environments before production?
  • 0
    You should have a mapper (a simple dictionary) to automatically pass the bool and get the value.. I would feel more safe with that
  • 0
    @Hastouki I don't know about them but we have test environments but do not write unit tests and do joy have QA testers. So...this seems soooooo typical to me.

    :'(
  • 0
    He saved more than just your job.
Add Comment