Good code review has a lot of benefits. Walking through a pull-request is a vector for knowledge transfer. It’s a tool for detecting and fixing issues early and maintaining consistency across a code base. It’s not easy to do well, but giving thoughtful and constructive code review is one of the most valuable skills you can develop.

What to ask

Frog and Toad by Arnold Lobel Frog and Toad by Arnold Lobel

First, consider the requirements. Every bit of code should be there because it gives something to a user. If the code adds a feature that someone wants, works, and has tests, the bias should be towards merging. Later PRs can always refactor and generalize, assuming continuous deployment.

Is it the right scope? Ideally, a pull-request should add a bite size piece of functionality - a single feature, a bug fix, or a refactoring, but not all three. Refactorings are cleaner if no new code is added at the same time.

You’ll have a gut feel about how well new code fits into an existing code base. Does it deal in the same abstractions? Does it reimplement existing functionality? Is the new code immitating deprecated patterns? Or not following existing patterns that are good? If the PR clearly adds technical debt, make sure that’s warranted. Note the decision in the PR and create a ticket to resolve the quick and dirty parts while it’s fresh in mind.

Here’s a checklist I put together as a reminder to myself. Maybe it will help you, too. You’ll notice that several of the bullet points below contradict others. If you’re spotting such trade-offs and weighing them, that’s judgement.

Checklist

  • Fits requirements
  • Clarity, readability, naming, comments
  • Complexity, simpler is better
  • Duplication
    • Don’t reinvent the wheel; make use of libraries and in-house components.
    • Finding overlap with existing functionality? Factor out common code.
    • DRY (don’t repeat yourself)
  • Abstraction and modularity
    • Does the code live in the right place? In the right layer?
    • Minimize surface area and cognitive load
    • What implementation details do we want to hide?
    • What level of generality are we shooting for?
  • Extensibility
    • Consider extension points for likely vectors of change
    • Also, consider that you ain’t gonna need it
  • Efficiency and performance
    • Choice of data structures
    • Memory and cpu usage
    • Look at inner loops, repeated work
  • Testing
    • Reasonable test coverage: happy path and edge cases
    • Is code factored into small testable units?
    • Look for missing test cases
    • Test properties or invariants
  • Dependencies
    • Fewer is better
    • Watch out for circular dependencies
  • Error handling
    • What could go wrong and what should happen when it does?
    • Fail fast
  • Clean up
    • opened resources get closed; use finally
  • Logging
    • Log errors and very sparsely to INFO.
  • Security
    • Least privilege
    • Sanitize inputs; look for injection
    • If I was trying to steal data, how might I exploit this code?

Style points

Idiomatic use of a programming language results in elegant and efficient solutions. Idiomatic style can be a great topic for code review, lifting intermediate skills toward expertise.

On the other hand, don’t fight the style wars. We all love beautiful code. But my aesthetic isn’t necessarily yours. You can convene the committee to write the style guide and zealously flame-broil your colleagues over every violation, or adopt standards like gofmt or Black and forget about it.

Gunnar Morling’s Code Review Pyramid gives a good indication of where to focus your efforts.

What is good code?

What makes code good or bad? What does it mean to be well-factored? There’s a lot of advice out there, most of it well-intended, some of it occasionally helpful. Popular heuristics include:

  • SOLID
  • YAGNI
  • Code smells
  • Be conservative in what you send; be liberal in what you accept
  • Principle of least surprise
  • Favor composition over inheritance
  • Code to an interface

Code Complexity vs Years of Programming by @flaviocopes Code Complexity vs Years of Programming by @flaviocopes

Simplicity deserves special mention. A simpler implementation will allow us to deploy working code and start getting feedback sooner. It costs less to maintain and probably has fewer bugs. Simplicity and generality often go together. We can always iterate based on feedback to fix rough spots and add features. You won’t regret giving a listen to Rich Hickey’s thoughts on the topic.

Programming languages provide primitives, means of combination, and means of abstraction, according to SICP. When looking at any piece of code, take note of which of the available means were chosen and why. Immutable data and pure functions are easy to reason about and should generally be our first choices. I write way fewer classes than I used to. Your mileage may vary.

Give Props

As developers, we’re proud of our work. Executives, PMs, and customers don’t care about pretty code or a clever hack. So, it’s nice when someone gets it. A little positive energy goes a long way. Don’t forget to give your fellow code-monkey some props.

Other code review checklists