Reviewing Code

Reading

  • Thoughtful code reviews: as practiced at Gitlab
  • Conventional comments: is an idea I like a lot. Make your intent obvious when you say something about code and include as much context as you can
    • eg suggestion (security): I think there may be an issue here …

Code reviews are a most excellent opportunity to share experience about a program or the way a team thinks about work! The goal is to share context, and potentially relevant personal experience with the author about a problem / solution, and to spot things that could bite you. Nitpicking is not helpful!

The Review

  • Does it work? 😆
  • Show me the tests! Seeing tests brings me joy! 🥰
  • Is it easy to understand and written simply? Avoid the clever. Readability is super important. This can’t be overstated
  • Focus on the interesting bits. Eg:
    • Concurrency
    • New libraries / dependencies
      • What’s the license? LGPL and Apache licenses are acceptible (Others could be onerous and should be scrutinized. Avoid copyleft …)
    • Error handling (See below)
    • Remote calls
      • What if your dependency isn’t available or slow to respond? Have we thought about the failure case? (Timeouts [both connection, read], circuit breaker)
      • Retries if it makes sense (idempotent action), and backoff
    • Db usage
      • Can we be caching here
      • Do we have / need an index for this? Is the new feature on a hot code path?

Error Handling

  • Are exceptions being captured and an error report being created for issues we can do something about?
    • Yes – bad configuration in a client, or module leading to failure
    • No – bad user input (eg Somebody entered a birthdate in a form not in the right format causing our input validation layer to fail)
  • Otherwise log (Or maybe log in addition to?)
  • Handle specific exceptions you know may be thrown, and include a generic handler for everything else
  • Propagate errors out to the places where they can be handled

My favourite code review links