A few weeks back while I was having very interesting conversations with colleagues about code reviews I sat down and wrote some topics on how (and why) I take them on.

There are already a lot of awesome articles online on what you should be on the look out when reviewing code — such as this great one by thoughtbot — but my objective on this exercise is to focus on improving the code reviews themselves, not on specific code patterns.

Furthermore, most of my notes below are focused on how I like to approach them from the submitter side---not the reviewer!

So, without further ado, here are my thoughts on code reviews, in easy-to-digest bullets:

Part I: Before submitting a review

1. Commits tell a story

  • The more complex the change, the more important is to tell a story

  • Commits and commit messages SHOULD explain the why in addition to the what

    • Aimed at other team members and your future self

    • Can someone explain why in 6 months by reading your message and commit diff?

  • If commit is not interesting to the story, feel free to squash it

    • For instance: "fix typo in variable name"

    • Be careful about squashing and rewriting commit history AFTER code review starts ➡ may be hard for reviewer to follow

  • Feel free to rewrite the way you tell the story before you open the code review

    • Don’t worry too much about it during development on your machine

2. Review your own code

  • You have the most context on the change

  • Be critical:

    • Is this part really needed?

    • What would you improve (even if not right now)?

    • Are there unrelated changes?

    • Are there accidental changes?

      • (Hello temporary files, whitespace and random IDE changes)

      • .gitignore (which can be global for a number of repos!) is a great help here

  • Great way to avoid minor comments and to avoid discussion getting too lost in them

3. Have a computer review your code

  • All tests green

  • Coverage

  • Linting

  • Other tools?

    • Mutation testing?

    • …​ what else?

  • But! Be careful about impact during development. These tools should not impede debugging/experimentation and only need to be clean/all green before the review is requested

4. Be mindful of the diff and diff noise

  • Be careful about renaming/moving lots of things, and then fixing one or two lines in the process:

    • Consider renaming first, and then fixing (remember: tell the story)

    • Consider making separate code reviews

    • Consider how easy is it to review a 2k line code review with renames + some sprinkled changes VS…​

      • Code review #1: 2k lines changed, only renames/moves made with IDE or search and replace; no other changes

      • Code review #2: 10 lines changed, "fixed foo"

  • But! Don’t be afraid of constantly improving codebase

    • Be careful of "let’s not touch this to avoid diff noise". Do touch it! Just submit it on a separate code review or commit! 😄

5. Be mindful of the reviewer’s time/effort

  • Avoid skipping steps that you could have done (such as re-running tests) and relying on the reviewer to pick it up

    • Unless agreed in advance: Team may decide to help unburden the submitter by being more active and involved in the review (may even pick it up and carry on in her stead)

6. Can you feature flag it?

  • Working on your own branch for a long time is a pain: there’s going to be conflicts, painful rebase operations, concerns about merge/deployment timing, etc

  • So, can you feature flag it?

    • Having new code disabled allows you to break something down into small incremental steps without impacting code stability

    • Smaller reviews, merge often

    • Only enable when ready and tested

  • It’s ok to develop in a branch, if they are short-lived

    • Definition of short-lived left to the reader and team ;)

  • Be careful about leaving behind old code after enabling feature flag

    • Feature flags should be short lived and have clear objectives/targets for when they should be removed. Otherwise they are probably options

    • Consider opening a pull request that removes the feature flag that will stay open and only rebased/merged down the line — it’s a great exercise and often reveals hidden dependencies on the old code

7. Be comfortable enough to press the merge button immediately

  • If not, say it. Call it RFC/WIP. Tell people what’s missing, and what you want feedback for

  • Otherwise people will (and should) assume you’re happy with the current state of the work, and that it does what you set out to do

8. Remember that a code review is not something you do after you finish a task

  • It ➡ is ⬅ part of the task.

Part II: During a review

9. Continue telling a story

  • New commits should still tell a story

    • Avoid big "fix issues pointed out during review" commits

    • Although a small "fix typos/improve naming" is ok too

10. Lengthy reviews with lots of churn: upstream issue?

  • Code review is a great safety net. Sometimes it catches bigger unexpected oversights from previous steps

  • If code review is taking long and a lot of changes are being discussed:

    • Maybe the design should have been fleshed out better before development?

    • Maybe some mentoring / pairing was needed?

  • It’s ok for this to happen. Quite insightful to look at why and try to improve for next time

11. Clarify if feedback is blocking or not

  • This is both on the reviewer and on the submitter:

    • Is proposed change a must-have?

    • Can some addition be pushed to a separate code review?

    • Is it just a suggestion?

12. It’s ok to go back!

  • Feel free to close/split/cancel your code review, if you believe it will have a positive result, such as speeding up the review process

13. Answer reviewer’s questions with code

  • From the Google Testing Blog: "Don’t just reply --- make the code easier to understand or add a comment. Someone else is going to have the same question later on."

14. Can a reviewer’s comment apply elsewhere in this code review?

  • A useful comment/insight quite often may apply to multiple places in the same code review

    • Do a quick pass over your diff and check if you fixed all similar instances

Part III: During a review — for reviewers

15: Build it in your head

  • Go beyond "looks ok"!

16: Imagine how you’d make this change

  • Be careful not to get too attached to your opinion though. But do compare it with solution you see

17. Prioritize code reviews

  • Try to review ASAP

  • Try to reply to feedback ASAP

  • Timebox if needed

Part IV: Closing the review

18. Trust your team

  • Place trust on your team-mates! If remaining changes are small enough, it’s ok to pre-approve the code review and rely on the submitter to push through the final version

    • Instead of requiring a new re-review just for small things

  • If the submitter decides to do bigger changes, it falls on her the responsibility to call them out and ask for a re-review

19. Shared responsibility for 👍

  • Be comfortable about giving your "LGTM 👍", pressing merge, and then going home while you’re on call

    • It’s everyone’s responsibility to make sure the change is in good shape

  • But…​ Do balance this out with getting shit done: It’s ok to 👍 even when there are concerns on something WIP as long as this is clearly identified, communicated, and of course there is no negative impact on the users of the service/codebase

20. Remember that "it’s not done until it ships"

 — Steve Jobs

21. Guidelines, not rules

  • The above ideas are guidelines that support

    • Communication

    • Learning

    • Collaboration

    • Speed

    • etc…​

  • The end goal is delivering a better product through engineering excellence

  • But do skip/ignore/replace any ideas that are not having a positive impact towards our team’s goals

22. Team decides best practices

  • It’s up to all members of the team to decide what to allow or not, decide what we want to enforce a set of rules (and which rules) or not, etc