I am the only developer in my codebase right now, so I do all of my own code reviews. I post a PR, and then go review it. It goes quickly—I rarely have to wait.

But this is not what I’m talking about.

If you post a review, and the reviewer finds a lot of problems, that’s not because they are good reviewer. If QA finds obvious bugs, we know it’s because the work is bad. This is the same thing. Code reviewers and QA should not be finding obvious problems because there shouldn’t be any.

One reason this happens is that we feel pressured to get work done quickly, and so we post PRs that are not finished. The code can pass a simple review, but perhaps it doesn’t deal with edge cases or does things in a sloppy way with no unit-tests or is convoluted in a way that works, but is hard to read. Maybe the code is AI generated and PR’d after a cursory check.

The problem in each of these cases is not that AI generated code or that code was convoluted or that tests weren’t there. It’s that the author didn’t review their own code and fix these issues before opening the PR.

So, the author got their task done quickly, but now the code review eats up all of that saved time. It also obscures the problems that could have been found if the reviewer hadn’t gotten tired of making obvious comments. The author ends up spending more time, the reviewer is wasting all of their time, and all of this causes a big gap between calendar time and implementation time, which is a driver in incorrect estimates and low developer productivity.

The low productivity problem is insidious. It feels like we’re coding as fast as we can, but the unnecessary wait time builds up and makes projects late. Those wait times are rarely taken into account in an estimate—nor should they be. Instead of accounting for them, you should be eliminating them.

So, don’t post PRs of unfinished work that is going to cause the PR to be sent back (or worse, cause the ticket to be reopened). The best way to do this is to do your own code review first, fix all of the problems you find, fix up the commits, and then post the PR for a real review. By that time, the code should be correct and easy to see that it’s correct. That means:

1. You ran all of the automated checkers/formatters already and they all pass (maybe even ones that are not in your CI).
2. You tested your changes (see diff-cover to filter code coverage reports to your branch’s changes)
3. You tested edge cases
4. If you got to a solution with convoluted code (which is totally fine), you did a refactoring pass to make it easier to understand
5. If you generated code with AI, you did your own editing pass on it to fix obvious problems (like distracting comments).
6. You did an AI assisted code review of your own code and thought about each issue critically. Collect examples of your past reviews that got comments that should have been avoided and use them to prompt your AI reviews (to avoid repeating mistakes).
7. The PR commits are created carefully to be easy to review one-at-a-time. So, whatever problems you found are not added on commits, but squashed to make it look like the code was written correctly to begin with. Don’t make a reviewer read code in one commit that is fixed or removed in a subsequent one.
8. If you know that there are things that are unfinished in this PR, you explained that in your PR description and when they will be done.

Do this for your own enjoyment at work. When you find your own problems, it makes you feel good about yourself and boosts your self-esteem. When someone else points out your flaws, it doesn’t.

For most of my career, code reviews were rare. They weren’t directly supported by the most common source controls systems I used (RCS, SourceSafe, CVS, SVN, and early TFS) until git. Branching was hard to impossible, so we just all committed to the main branch. As a lead, I would read a lot of diffs after the fact, but not all of them, and I rarely sent comments.

I did, however, adopt unit tests and CI relatively early—in 2000, right after I read eXtreme Programming Explained [affiliate link]. So every change was unit tested soon after it was committed. We didn’t do code reviews, but we did a reasonable amount of pair programming, which had similar effects. Frankly, even after adopting code reviews for every PR a decade later (and pairing less), I can’t say that software I worked on had noticeably fewer bugs. Seems about the same.

What code reviews did do was make sure that more people than the author knew about the changes that were happening. Code reviews were far more efficient than pairing, and could be done asynchronously. I did code reviews nearly every day I worked on Trello, and I never worked on a team codebase I felt more generally comfortable with.

I think the defect-finding part of code reviews can be done by AI or other automated tools better than most humans—certainly faster. It’s not easy to see bugs just by reading code unless it’s egregious, and the automated tools are great at finding the egregious errors. Type systems and unit tests are also better than most programmers.

But, the information sharing part—that’s the real reason I think code reviews are important. I recently wrote that legacy code is code we didn’t write or review, so we shouldn’t let automation take over this part yet.

Right after I read eXtreme Programming: Explained [affiliate link], we built a CI system at Droplets (where I worked). CruiseControl didn’t exist yet, so anyone trying to do this just figured it out for themselves. Our system was based on a physical build token.

All of our desktop machines were named after movies—the CI Build Machine was named Goodfellas. I printed out an 5×7 poster of the movie and put the manual instructions for doing CI on the back. Then, I laminated it. You needed to have this card on your desk if you wanted to commit code.

Here is what it said on the back of the card:

1. You must have this card on your desk to proceed
2. Commit your code
3. Go over to Goodfellas (don’t take the card)
4. Get your changes
5. Run the build and test scripts
6. If it’s broken, fix it
7. If you know someone was waiting for this card, give it to them
8. Otherwise, put the card back where you found it

The build and test were automated, but you did need to manually grab the card.

Doing this solved our problem of broken builds caused by “it worked on my machine” problems. Seems antiquated now, but before easy branching and merging, this one-at-a-time verification caught a lot of problems early.

Three years ago, I identified five pieces of software that I thought were “great works” that were written by one or a very few number of people. In rough chronological order, they are C/Unix, The ARM instruction set, TeX, VisiCalc, and the Web Browser/Server. I didn’t pick them based on what the software did, but they are all basically programming languages. They also are somewhat old (> 30 years), yet still exist or have a lineage of derivative projects (i.e. they were not lost).

Since then, I thought of two others. They are also programming languages of a sort and both are enabled by the existence of the web. Both are in heavy use, albeit mostly through derivatives. They are also both relatively tiny programs—more like poetry than prose.

The first is Markdown by John Gruber—you can download the original Perl at that link. Markdown is ubiquitous and finished, in the sense that it hasn’t been updated, and it doesn’t need updates.

Even the derivatives don’t need updates. In 2016 I wrote the Swift implementation of Trello Flavored Markdown by porting our web client’s JavaScript version (so it would match exactly). It was a little longer than Gruber’s original, but not by much. Once it was done, it only had one update that I know of.

The second software poem is the original Wiki software by Ward Cunningham. I don’t know if the original source is available, but I used this Perl implementation he published when I wanted a locally hosted version—it was my original second brain. I use Obsidian for this now, which (of course) is based on Markdown.

It’s funny that both Gruber and Cunningham chose Perl to write their poems. Perl syntax lends itself to writing actual poems—I have a vague memory of seeing some in Larry Wall’s Perl book. I would also note that both poems have minimal dependencies. I don’t think Markdown uses anything that isn’t in Perl. WikiWikiWeb uses Berkeley DB by default, but I trivially replaced that with simple files in my version (which I can’t find, but I do have the files, and they are just text).

They were also built in a time when it was common to build web server software in Perl (Markdown is 2004, WikiWikiWeb is 1995). I learned Perl in 1993 at my first job, which we used to replace all of our shell and awk scripts. I built production websites with Perl until 1999, but Perl was still my go-to language for scripting until 2013 when I switched to Python for web backends, scripting, and anything where Pandas would help. I’m sure Perl has equivalents for everything I do, but I never learned modern Perl.

My attempt at a poem of this nature is Page-o-Mat, a YAML-based language for making journals. It’s small, but I need YAML and PDF dependencies to make that happen. That fact about modern programming makes me think we won’t see another great software poem again.

This is part of a series on shortening PR code review time. In the first post, I recommended gathering metrics (see If code reviews take too long, do this first). If you base a goal on these metrics, it’s possible that that metric will be gamed. This is not because of malice—it’s a well-known phenomenon (Goodhart’s Law), which just recognizes that it’s human behavior to make a metric improve if leadership says that it has to.

So, if you set a goal to lower the total time from PR creation to merge, then that time will probably shorten. You are hoping that we do this the right way by reducing bad comments and pre-checking PRs. But it’s inevitable that some improvement in this metric will be caused by behavior that you don’t want. For example, some bugs that could have been caught in review will make it to QA and production. This is the tradeoff if you want faster reviews—slow reviews are expensive too, and they are only worth it for expensive defects. Even if we agree that it’s cheaper to find problems in review than QA or production, there is still a cost where we’re fine with it being found in later (or never). But, if expensive issues are being found in production, it’s worth checking if that‘s because reviews are not thorough enough.

One way to make metrics less likely to be gamed is to pair them with a compensating metric that measures the effect of the gaming. So, in a call center, you might measure the time spent on a call, which you would want to decrease. To make sure that customer problems are still being solved, you might pair that with customer satisfaction scores or the number of times a customer has to call back. You want the pair of metrics you choose to both improve, not for one to improve at the expense of the other.

This is why I recommend pairing testing code coverage with some other metric that makes sure you get coverage of the most important or most risky code. For code review time, you might pair that with the DORA metric of “Change Fail Percentage” (percent of deploys with a bug requiring a hotfix or rollback). The pairing of metrics should imply a set of behaviors that can improve them both.

You could also go a step further and track the behaviors (or lead indicators) and not just the lagging indicators. A good lead indicator is a behavior, not an outcome, so it’s harder to game. For example, to make it more likely that a PR has a low idle time and is easier to review, it should be small. We don’t want to get into line counts, so just a high-level indicator of “excellent”, “acceptable”, and “unacceptable” PR size that is based on a fuzzy combination of the number of commits, changeset size, and maximum commit size could be enough. Just giving this indicator back to the author before they PR (with a local script) would encourage them break down their PR’s. If leads see undesirable behavior sneaking through, they could augment the script or override its assessment and reject the PR anyway.

Paired, complementary metrics are harder to game because the combination limits behaviors to those that can improve both metrics. A leading indicator is harder to game because it’s happening in real-time and you can react to it. So use these techniques to make sure your metrics are showing improvement in the underlying thing you care about.

I’ve been writing about code reviews for the past week because I think that shortening review cycles is one of the easiest ways to increase developer productivity. But I’ve taken that as a given so far because I’ve seen it on my own teams. A few years ago, I saw that the relationship between productivity and things like code review time has been supported by research, and I continue to see it in my consulting practice.

At Trello, I worked on the iOS app. We were a team of ten engineers who worked closely with the ten engineers building the Android app. We shared managers, PMs, designers, roadmaps, and thought of ourselves as the mobile team. We had shared processes, but since we were somewhat insulated from the other teams, with less intermingling, our processes were different from the rest of Trello.

At some point, a (non-mobile) engineer decided to gather metrics across all teams to see what he could learn about code reviews (see If code reviews take too long, do this first). He plotted various statistics and presented them for discussion. In slide after slide, the two mobile teams were outliers on nearly every statistic. The four metrics that stood out were: the size of the PRs, the number of PRs, PR idle time, and the total time to approval. Both mobile teams had the smallest and most PRs (normalized for team size). We also had the shortest times, which I think is a consequence of the first two metrics.

I can tell you from being on the mobile team in that time, that this was intentional and built into the mobile team culture. I was trained on our PR culture on my first day, when the entire mobile team was about five engineers, and we kept passing that on to new team members (see Construct PRs to Make Reviewing Easy). The short feedback loops were enabled by our careful PR construction and pre-checks, which I documented in PR Authors Have a lot of Control on PR Idle Time. When we dug deeper into the code review data, we found that the most senior iOS engineer and the most senior Android engineer were modeling that behavior (short and frequent PRs), which set a tone for the teams.

The shorter loops also contributed to my personal happiness at work and feeling of productivity. It was only after I left the team that I saw DevEx’s identification of feedback loops as a key driver of productivity. In the three years since I left, I’ve been seeing code review feedback loops as the easiest loop to shorten. It also has a large and immediate impact because it affects every engineer nearly every day.

According to DevEx, short feedback loops help by reducing context switching and enabling rapid course correction. I have seen that they also align estimated time with calendar time, which is the main source of the discrepancy in the feeling of productivity between engineering and everyone else (see Confession: I Estimate Using Time).

Some of the effect of shortening loops improves productivity without requiring us to “work faster”, but its effect on context switching is different. Reducing context switching does make us work faster. If we have to wait for days for a PR to be approved, then it takes longer to get started on the next step. We might have had to work on something unrelated in the meantime. That switch back to this PR (which might require review responses) causes a delay whose length is determined by the length of the waiting time. Having shorter code review loops means that we probably still have enough of the context in our heads and can respond faster.

So, reducing the wait time for a code review to start (PR idle time) and the total time it takes to approve a PR both reduces calendar time and context switches, which in turn reduces the time it takes to respond to a code review and to move on when the PR is part of a larger project.