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? 😆
  • Is it designed well?
    • Does it belong where it’s being added? (Would it simplify life or be more appropriately put somewhere else? eg A library or different app?)
    • Is it consistent with the rest of the program it’s being added to?
  • Spend more time looking at the interesting bits. Some examples of interesting:
    • 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, retries, backoff)
    • Db
      • Can we be caching here
      • Do we have / need an index for this? Is the new feature on a hot code path?
  • Is the change easy to understand and written simply? Avoid being clever.
  • “Show me the tests!” (Units, functionals, end-to-ends, or whatever else might be appropriate. Seeing tests brings me joy 🥰)
  • Do we need to update docs? (internal or external)

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)
  • Sending an error to a log file means it probably won’t be noticed unless an investigation is underway. Mostly informational. Log events might be used during investigations but they’re not a way to hear about something that’s broken in the first place
  • Noise: Defend the channel we receive errors on because too much noise leads to fatigue, then apathy. Given the way we’ve setup error handling in this code, how many reports do we expect to receive for a given condition? And are they actionable …

My favourite code review links