💬Code Reviews

My approach is shaped by my experience working on small teams within a small organization without a formalized code review structure.

Goals of Code Review

• Keep the codebase coherent for all contributors, current and future:
  • Choose (and document) shared styles, patterns, and conventions and adhere to them whenever possible
  • Have multiple people examine new variable and function names for clarity
  • Have multiple people examine logic and organization for code readability
• Help the whole team stay up to date on application changes
• Facilitate team learning by reviewing each other’s approaches to problem-solving
• Leverage more team members’ experience in anticipating edge cases or likely complications from a change

Best Practices as a Requester

• Include a link to your ticket, or a summary of requirements, as agreed by your team.
• If you’re working in a language or framework that’s new to you, communicate that to your reviewers ahead of time if possible — this can help reviewers calibrate their comments.
• Make sure your code is really ready for review. This means:
  • Test, lint, and update any relevant docs before requesting a review
  • Think about and check for edge cases and forgotten implications of incomplete requirements
  • Review task acceptance criteria and confirm they’ve been met
  • Make sure you’ve cleaned up unnecessary comments and TODOs
• Proactively leave comments if you had to include something you’d question as a reviewer (e.g. a hacky choice you couldn’t find a way around, or incomplete acceptance criteria that had to be split into a new ticket, etc.). Worst case, you’ve explained your rationale; best case, your reviewer might have another idea.
• Keep an open mind about feedback and make sure you understand the why behind the request so you get something from it. (And if you disagree with a comment, explain the why behind your solution.)
• Change requests and constructive criticism are part of the learning process. Take a walk before replying if you start feeling annoyed or defensive. 🙂
• Balancing code quality and delivering real results for users is a critical part of this job. If addressing change requests is threatening your deadline, talk to your reviewer to prioritize blocking vs. non-blocking changes, and keep your engineering lead and/or project manager in the loop about scheduling impacts.

Best Practices as a Reviewer

• Always remember we are all people, not machines. People are not perfect. People have feelings. People take pride in their work and get sensitive about criticism. People struggle and grow. Be excellent to the people on your team. It’s the human thing to do, and building trust and good feelings has compounding benefits for your project.
• On a small team without a documented process for code review, I like to talk to individual team members about their experience level with different tools we’re using and how they prefer receiving feedback. Meeting people where they are can help keep the experience positive and productive for everyone.
• I focus on asking questions to understand the approach (unless it’s obvious, which is always great). This is nice because it helps me see how another dev thinks about a problem, and if there’s a problem with the approach we can work through it together.
• If there’s a clear problem, suggest a solution. Don’t just point to what’s wrong, and don’t leave someone guessing what you want from them. Explain the thought process behind your solution.
• I think the most important things to keep an eye on through code reviews are consistency with project patterns, organization, systems, and documentation. Maintainability. (On the subject of project patterns: don’t assume another dev understands the pattern you’re using or the reason you chose it. Talk about it. Document in a style guide. Be prepared to teach. We’re all learning.)
• When I’m working with less experienced devs, I look for appropriate opportunities to dry up reused logic or teach an easier way to do something.
• If I’m going to flag a potential problem, I explain the “why” — a past experience, unexpected side effect, etc.
• It’s not about “right vs. wrong.” (Even if something really is wrong.) That’s just an unhealthy attitude for a team. It’s about collaboration, keeping everyone on the same page about changes that are coming into the codebase and why we’re choosing a particular solution, and keeping the codebase maintainable. In the end, building a strong team that works well together is way more powerful than any individual line of code.

Resources

• Helpful code review comment guide
• And the post that sparked it
• Justin shared this ebook about productive code reviews