[The mythical code review](https://remysharp.com/2021/03/24/the-mythical-code-review "Permanent Link: The mythical code review")

It’s easier to determine what a code review is not:

So… what exactly is a code review good for? If there’s a QA process in place, and tests and coding styles and linting, what exactly is the point of reviewing?

I think, a code review is useful only as a way for someone else to know that a change occurred and roughly where it occurred. It should go without say that it’s useful to spread knowledge of code around more than a single person.

But that’s kind of it.

However, if I really have to code review, what works?

A code review for a handful of lines of code is my kind of perfect. It’ll often be for a laser focused pull request that resolves a very specific issue.

During small reviews it’s possible to grok the code and functionality from these lines (effectively "imagining" how the code will work).

Large code reviews are pretty worthless on their own. I’ve started to take the approach that when I’m the author of the large code reviews, that I add comments throughout in the pull request.

Which begs the question, why not comments in the code? Because comments in the code explain the why of the code (or something that’s not immediately obvious), whereas comments in the pull request, against the specific lines of code in a large(r) pull request adds information that’s specific to the code review.

We’re talking multi-developer, hundreds of lines of code - it’s not a code review. At best you’re going to get someone to scroll the complete length of the files but it’s not going in.

There’s a couple of options here:

Generally though, avoid this - it doesn’t help the author or the company and it can lead to some real discomfort around code reviews if it happens often.

I’ve included the following as a cautionary tale to developers today. I pray you’re not going through a similar process, but if you are - it’s not a sustainable approach.

My first exposure to code reviews was for the company I worked for from '99 through to 2008. The standard process was that developers in the team would collect every file they had touched and add the full file name and path to an Excel spreadsheet. This would happen on the afternoon of a release, and we’d often have multiple forks of work going on so there’d be multiple releases per month.

This was the days of CSV (which predates SVN which predates Git), so the revision control system was, comparatively basic to what I have today.

I would download all the production code and then individually diff every single file. Quite often a release would involve well over a hundred files, modified by four or more developers.

Automated tests weren’t part of our business back then so my eyes were the last pass before the code would go live. Hundreds of files, hundreds more lines to diff without any real-time context.

I remember it would take me hours. We laughably upgraded the process so that team leads (as the team grew) would run the diffs and "if they was too much to diff, another team lead would help" - like we all didn’t have enough to do. At no point did anyone step back and ask: why are there so many files in a single release?

So the release would go live. Inevitably something would break (large releases + no tests = sure way to break things) and the question would be asked: Remy, why didn’t you spot this bug?

I wish I had the experience I have now to reply: because I just diffed over 500 lines of code and it was a single line bug - you need to invest in testing and not relying on tired eyes at the end the day to hopefully catch obscure bugs.

With the next 20 years experience I would learn not only what works, but also learn to have the confidence in being able to push back against bad practices.

Where do you sit on code reviews?

Published 24-Mar 2021 under #web & #business. [Edit this post](https://github.com/remy/remysharp.com/blob/main/public/blog/the-mythical-code-review.md)

👍 5 likes

[](](](https://twitter.com/benjaminbenben)

Lock Thread

Login

Add Comment[M ↓   Markdown]()

[Upvotes]()[Newest]()[Oldest]()

[Dominic Mitchell](http://happygiraffe.net/)

0 points

2 years ago

I hated code review before joining Google (about a decade ago). Since then I’ve found it to be utterly invaluable. In part this is due to the situation at Google: we have a monorepo and good tooling to support code review within that. Looking at the stats I think I’ve done \~15k reviews in this period.

However, I completely agree with most of what you’ve said above. The key bit is that it’s about human interaction. I’ve really valued the discussion about making the system better. I’ve learned huge amounts through interacting with other engineers who know different things to me. The code I’ve worked on is better understood by my team because it’s been reviewed by them (higher bus factor).

Google’s code review guidelines have been published: https://google.github.io/eng-practices/review/

[Pat O’Neill](https://www.misteroneill.com/)

1 point

2 years ago

I am completely on board! This is a topic we’ve discussed at length on my teams.

I think I get what you’re saying here: that "changes that could be made" are suggestions that are out of scope of the change.

The other interpretation of "changes that could be made" would be discussion of alternate solutions - i.e. changes that could be made to the changes. That’s one of the most important parts of code review for me:

Also, code reviews can be a great way for new folks to learn the codebase. We have a lot of code that spans many repositories and, for our product, at least, some as old as 8+ years. If we have to onboard someone, we try to encourage them to perform lots of code reviews precisely because they aren’t familiar with the code and are not as blind as the old hands to some of the dark corners.

[Commento](https://commento.io)