20 questions for a valuable code review

Published on December 28, 2021

This is foundational series article. Read more here

Tagged: #engineering

Follow me on twitter for more posts like this

I recently had an interesting discussion around the value of doing code reviews and the value of mandatory code reviews.

I think code reviews are extremely valuable and should be done by most organisations and teams.

A valuable code review will

  • pass institutional knowledge around the org
  • help all engineers grow their skills
  • maintain quality in the face of all the other time pressures your team faces

But how do you keep a code review valuable?

  1. Automate instead of PR checklists
  2. Focus on code readability and functional correctness

Automate instead of PR checklists

Anything that can be automated must be automated. Engineers are expensive! Don’t waste your time on code formatting or spell checking if at all possible.

It’s OK to have some checklists in a PR template but you should always be actively working to remove checklist items through automation or engineering culture.

20 code readability and functional correctness code review questions

  1. Is the intent clear?
  2. Is there needless duplication?
  3. Could existing code have solved this?
  4. Is this an atomic commit?
  5. Could this be simpler?
  6. Are there obvious logic issues?
  7. Does it need tests?
  8. Are the tests comprehensive and clear?
  9. Are there any security issues or considerations?
  10. Are there any accessibility considerations?
  11. Are there any PII considerations?
  12. Is it instrumented? Are there relevant analytics? Are there relevant production logs?
  13. Are there revert scripts for database changes?
  14. Is the database work ACID? Is it transactioned sensibly?
  15. Does this feature need to be turned off in the future?
  16. Is it easy to turn it off?
  17. How easy is it to delete the feature code as a whole entity?
  18. Does the code respect Command-Query separation?
  19. If DDD - Are there well defined aggregate roots, value objects, enumerations?
  20. Will there be any issues recovering from a transient failure? Is the solution distributed in an unexpected way?

A note on “nits”

In general I do not look for coding style, syntax I don’t like, styling or spelling errors in a code review.

If these things are important to your team then just use your CI system to detect and fix issues automatically.

In typescript/javascript we have ESlint, Prettier and Vale to detect and fix issues automatically. There will be similar tooling for your language and framework of choice.

Consistency IS important. If an entire code base is using some syntax but you prefer a different syntax that is functionally the same, you should use the existing syntax unless the entire team agrees with you and there is a plan to change the code everywhere.

A note on mandatory code reviews

The PR system developed by GitHub was for Open Source contributions by distributed teams. Mandatory PRs might not be relevant for a team with consistent levels of engineering capability and knowledge of the system.

I do feel that if code reviews are mandatory it means they won’t be bypassed when the pressure is on.

When the team under pressure is when you really need code reviews the most. If some software is forcing one positive code review then a single developer can’t be pressured into putting dodgy code out to customers.

Summary

Code reviews that focus on finding nits, syntax you don’t like, spelling errors and things like that are not a valuable use of your time. Automate these with tooling instead.

Code reviews that focus on code readability and functional correctness are extremely valuable to the entire organisation, long after you have moved on to another project.

If you don’t enjoy code reviews, try changing their purpose and start evangelising what a good code review looks like.

Darragh ORiordan

Hi! I'm Darragh ORiordan.

I live and work in Sydney, Australia enjoying the mountains and the ocean.

I build and support happy teams that create high quality software for the web.

Contact me on Twitter!


Read more articles like this one...

List of article summaries

#developer-experience

How engineers can help deliver software effectively

Delivery managers and team leads have the responsibility to deliver a software system via an engineering team.

Your customer wants every feature to work perfectly and they want it delivered yesterday. Your team wants to learn and grow.

It’s a tough role managing all the stakeholders and creators in a project.

Engineers can help drive great delivery by empathising with and supporting the delivery manager or leads in a project team.

#engineering

Engineering systems for consistency and impact

Your most impactful engineering is done before you write any code.

It’s important to have some systems around how you approach problems to make sure you’re consistent every time.

These are some of the techniques I use to make sure I’m covering as many angles as possible when doing my pre-coding engineering.

#engineering

How software engineers can avoid commoditisation

Engineers spend most of their learning time on technical implementation content. Things like new frameworks, languages or cloud platforms.

But turning solutions into code is a tiny part of what you do and it’s getting less valuable year by year.

As we’ve seen with “no-code” and tools like GitHub Copilot, the implementation part of our role is increasingly becoming commoditised.

You could generalise that the value an engineer brings to a team is their ability to analyse problems and synthesise context. The part of your role as an engineer that will never be replaced by “no-code” or AI is this high level cognition.

The true human aspect of being an engineer is working in a team and considering other people’s ideas, emotions and thoughts while solving these problems.

So shouldn’t you train these meta-cognition skills as much as you train the specific technologies?

Every engineer should spend time learning and applying general tools for thinking. These tools are applicable to almost all problems so the compounded payback on your invested time is huge.

Clearer thinking will amplify all the other skills you have and any frameworks or tools you learn will give you results for the rest of your career.

Like any skill, improving the way you think takes deliberate study and practice.

These are some of the tools and systems for thinking that I refer back to all the time.