good-code-reviews.md

how to review code good and do other stuff good too

• A quick glance at the diff to spot anything obvious
• Pull it down and run it locally with a fresh install of all dependencies
  • open the console and watch for errors being logged
  • does this patch cover the AC in the story?
  • does this patch resolve repro steps in the bug/defect?
• Comb through code line by line:
  • Are there sanity checks in place (no infinite loops/recursion, etc)?
  • If there's complex or hard to follow code, can it be simplified?
  • Can comments be replaced with better variable/method names?
  • Are there comments where we need them?
  • Are the comments good?

    The proper use of comments is to compensate for our failure to express ourself in code.
    Note that I used the word failure. I meant it. Comments are always failures.
    We must have them because we cannot always figure out how to express ourselves without them, 
    but their use is not a cause for celebration.
    ...
    Why am I so down on comments? Because they lie. Not always, and not intentionally,
    but too often. The older a comment is, and the farther away it is from the code it describes,
    the more likely it is to be just plain wrong. The reason is simple. Programmers can’t realistically maintain them.
        - Robert Martin, Clean Code
    

  • Do the unit tests cover the edge cases that "will never happen"? That's where things break.
  • Do the unit tests cover our code or framework internals?
  • Does the automation cover the right things?

This is a living document. Feel free to comment on things you disagree with and add things that are missing!

Any discussion about reviewable vs. github probably starts on slack and ends in action; the discussion about code review expectations ends up in the dev wiki.

@mkj-pendo I totally agree reviewing backend code is going to be different from reviewing front end code. Perhaps we should break it out into "all", "front end", and "back end" sections.

As far as comments go - I agree completely that comments should explain the why and those comments do have historical value as long as they change when the why changes (we don't want comments to "lie"). I think the critique of comments is harsh to send the message that it's vitally important to make sure we review comments as rigorously as the code they explain.

It sounds like my "arguments against reviewable" mostly come from a place of ignorance. I don't mean to say "this is bad and we shouldn't use it", I'm only questing whether or not it really is a value add (and it sounds like it is, especially on the backend). I've "crossed out" the reviewable vs github debate - I think you're correct that it will muddy up the point of this gist.