Skip to content
guitton.co
Learn More

Code Reviews: a Cheat Sheet

  • #Data Leadership
Read time: 5 minutes

Code Review can turn into a weird Ping-Pong game
Code Reviews can turn into a weird Ping-Pong game

A code review is a process where someone other than the author(s) of a piece of code examines that code. Code committed to the codebase is both the responsibility of the author and the reviewer.

Done right, PR review can be the engine of team and business growth. Done poorly, they can leave the team fatigued and the business questioning. This guide is here to share my experience and best practices to avoid inefficient and unpleasant Code Reviews.

1. What to look atđź”—

A Code Review Maslow Pyramid of Needs
A Code Review Maslow Pyramid of Needs

Code reviews should look at:

  • Design: Is the code well-designed and appropriate for your system?
  • Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?
  • Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future? Beware of over-engineering. The code should solve problems that need to be solved _now_, and not problems that the code author speculates _might_ need to be solved in the future.
  • Tests: Does the code have correct and well-designed automated tests?
  • Naming: Did the developer choose clear names for variables, classes, methods, etc.?
  • Comments: Are the comments clear and useful? Note that comments are most useful when they explain _why_ the code exists.
  • Style: Does the code follow our style guides? Note, in most cases, style nits should be avoided and be enforced entirely by automated tooling. However, some stylistic decisions can be discussed if it impacts readability and complexity.
  • Documentation: Did the developer also update relevant documentation?

All of the above are grounds for a reviewer to request changes in a PR. Consensus should be reached to the best of the abilities of the author(s) and reviewer. However, if consensus cannot be reached between the two parties, the review should be escalated to the technical lead.

2. Code Review Conductđź”—

2.1. Be a great submitterđź”—

Provide context with the PR template

YOU are the primary reviewer

  • code review is not a tennis game where "the ball is in your court now". Review your code with the same level of detail that you would giving reviews.
  • make sure the code works
  • don't rely on others to catch your mistakes

Things to think about

  • did I check for reusable code or utility methods? is the code elegant?
  • did I remove debugger statements and prints? is the code readable?
  • is my code secure?
  • is my code maintainable?

Work in progressđź”—

We believe in starting a review early so you don’t get too far only to have to rewrite things after someone has made a great suggestion.

Just create a PR even with a readme commit (when 30 to 50% of the code is there, it's a good rule of thumb), and add a clear "[ WIP ]" tag to the title so that we know it's a work in progress.

The sooner you get feedback, the better: nobody wants to hear at 90% of the way "you need to redo everything".

Ask for review early and expect architectural design comments.

General Guidelinesđź”—

  • Provide context to the reader = use the PR template
  • Review your own code = if needed, build a sandbox
  • Expect conversation
  • Submit in progress work = see next slide
  • Submit reviews < 500 lines of python code
  • Use automated tools = see next slides
  • Be responsive
  • Accept defeat

How to allow maintainers to modify your PR

Allowing changes to a pull request branch created from a fork - GitHub Docs

2.2. Be a great reviewerđź”—

"Why don't you simply stretch and smile?"
"Why don't you simply stretch and smile?"
  1. Be kind.
  2. Explain your reasoning.
  3. Balance giving explicit directions with just pointing out problems and letting the developer decide.
  4. Encourage developers to simplify code or add code comments instead of just explaining the complexity to you.

Other details

  • Make sure you are aware of the problem/feature.
  • Don't be rude, be polite
  • Try to avoid the usage of the first person, try to talk about the PR, the Code, not about the author!
  • Give suggestions and make clear why do you think you suggestion is better than the current approach.
  • Link to resources, blog posts, stack overflow answers
  • Don't point out just the bad things, give compliment as well.
  • Ask questions instead of giving answers
  • Don't burn out: try to review max 400 lines of code in one session. Make it part of your daily workflow. (use github notifications)
  • Don't use the words "now simply", "easily", "just", "obviously" ...

What to provide feedback onđź”—

Code review is not only for experienced developers! Here is what you
can provide feedback on:

  • high-level business goals
  • high-level glance at the code and readability check
  • setup: can you run it ?
  • technical solutions / architecture design / the actual code

3. Engineering Managementđź”—

This part is closer to a manifesto than to anything else, but I still find it useful:

  • Universal code reviews: Everyone should review and be reviewed (junior or senior)
  • Ensure consistency:
    • We should agree on a style guide to move away from personal preference (we use Google's style guide for R and Python)
    • Once we agree on the style guide, start automating things with linters (from more painful to less: CI on the code, git pre-commit hooks or IDE setups for each developer)
  • performed by peers and not management (core review is not a performance review)
  • no blame culture

Referencesđź”—