Effective Code Reviews

Code that actually gets reviewed, and feedback that's actually helpful

David M. Lee, II @leedm777 GitHub @leedm777 Twitter [email protected]

Why Code Review?

One of the most cost effective ways to find defects[1]
Finds non-defect coding issues (maintainability, readability)
Promotes current best practices
Educational for all involved

[1]: Code Complete, chapter 20

Tools help, but aren't necessary

You can make git format-patch/svn diff + email work

Rules for effective submissions

“Look, that's why there's rules, understand? So that you think before you break 'em.” – Terry Pratchett

Post small reviews

Reviewer effectiveness drops off >400 LOC
Be respectful of the reviewer's time

Chart from SmartBear

Post even smaller commits

Don't lump several different changes into a single commit
- Put style/formatting changes into a different commit
- Break up into logical commits (things you fix along the way, vs. the intent of your submission)
git rebase and git commit --fixup is your friend

Write good commit messages

Commit history is the guide to why the code is the way it is.

How to Write a Git Commit Message

From Chris Beams:

Separate subject from body with a blank line
Limit the subject line to 50 characters
Capitalize the subject line
Do not end the subject line with a period
Use the imperative mood in the subject line
Wrap the body at 72 characters
Use the body to explain what and why vs. how

Use the imperative mood in the subject line

“spoken or written as if giving a command or instruction”
Examples (again, from Chris Beams)
- Refactor subsystem X for readability
- Update getting started documentation
- Remove deprecated methods
- Release version 1.0.0
Should complete the sentence “If applied, this commit will {{subject-line}}.”

Use the body to explain what and why vs. how

how is either implicit in your code, or belongs in code comments
what and why provide context; gives reasons for the change
- What did the code do before the change?
- Include references to relevant issues in issue tracker
  - Tooling can link commits/builds/issues

how is at best useless, or at worst misleading

commit 6ab909cdb8426c3c05c8ee253bd5002a77f3172b (HEAD -> throw-away)
Author: David M. Lee <[email protected]>
Date:   Wed Aug 16 13:44:32 2017 -0500

    Add 7 to x instead of 5

diff --git a/foo.js b/foo.js
index 50be92d2..a4ab74ed 100644
--- a/foo.js
+++ b/foo.js
@@ -1 +1 @@
-x = x + 5;
+x = x + 6;

why is actually helpful

commit 8c5bc990de82b9c5ec910e582643a9cd9c407bbf (HEAD -> throw-away)
Author: David M. Lee <[email protected]>
Date:   Wed Aug 16 13:44:32 2017 -0500

    Increase timeout increment
    
    When we need to bump up our timeouts (sadly in a variable named `x`), we
    weren't increasing it quite enough to account for slowness of WAN traffic.
    
    See XYZ-8373

diff --git a/foo.js b/foo.js
index 50be92d2..a4ab74ed 100644
--- a/foo.js
+++ b/foo.js
@@ -1 +1 @@
-x = x + 5;
+x = x + 6;