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!

One reason advanced for reviewable has been that it generally does a good job of handling comments across amended commits. We use that all that time on the back end. On the back end, the norm is that PRs are not accepted on their first review. I suggest that we entirely separate discussion of how to do a code review from discussion of what tool to use.

We will need to separate some things out in the how to review list by front end / back end. In particular the "run locally with fresh dependencies" is not applicable to our back end.

More on comments: comments should never say what the code is doing, only why the code is doing what it is doing, and only when there's a valid reason that the why would be ambiguous. My comments on the back end are preponderantly describing the purpose of blocks of test code, because in Go we aren't using a literate testing framework; the purpose of one block of mock/do/assert is not always distinctly obvious from the purpose of the next block of mock/do/assert and comments explaining the purpose make it easier to modify without inadvertently breaking the purpose when structural changes require such modifications.

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.