sdgluck

Code reviews are hard

Anyone who has worked in a software engineering team will be familiar with code reviews. They are the dogfights1 of the programming world. Perhaps that is too dramatic. We should not think of code reviews as fights, but sometimes they feel that way.

Communication in code reviews is key. Tone of voice is impossible to impart in written text. In my own experience as both the author and recipient of code reviews, text that is written as neutral is often received in a negative tone by the reader. This is a hard problem to solve. Maybe we need to use more emojis? 🙏 Or we need to learn to see positive.2

Over the years, I have been involved in many hundreds if not thousands of code reviews. I can't remember a day when I did not either submit changes for review, or review changes submitted by others. They become second nature, and as a result, it is easy to fall into bad habits.

Code reviews are first-class citizens of the Agile methodology3. Their purpose is broad, beyond that of simply ensuring code won't fall over in production. They also encompass ensuring that code works towards fulfilling team-, product-, and business-level goals and strategy. If the changes don't meet the requirements and standards of the team, they need revision. And the same applies to product and business; if the changes don't meet their requirements, they need revision.

As my responsibilities as an engineer have grown, so too has the scope of my work grown. I used to only wear one hat when reviewing and authoring code changes. My Developer HatTM. But nowadays I also wear my Business HatTM and my Product HatTM. It's hard to balance all of these hats on my head at the same time, so now I just wear my Code Review HatTM, which is an amalgamation of all three. Imagine the coolest hat you can imagine. That's what my Code Review HatTM looks like.

Code is 75%

Code reviews are hard work. It's our job as the author of code changes to make it easy for the reviewer to review them. The most important part of a code review is the context. And then after that, what the changes are. Why first, what later.

If I am reviewing code changes and they do not come with a description of why these changes are being made, I cannot do my job. There are many ways to implement a solution to any given problem, and lots of the time it comes down to opinion. I need to know why the author chose one solution over another, maybe there is a trade-off or business or product concern that I am not privy to?

For a given task, our job as authors does not end when we have finished writing code. It ends when the code is merged. When you open a PR, you are only 75% of the way there. The final 25% is code review and team work. Be a good team player and add a useful description that offers context which will help the reviewer.

It's a two-way street. Meet reviewer(s) half-way.

Bad code cascades

There is a difficult balance to strike when reviewing code. What should the reviewer look out for? In my head I have a few categories that I put code into when reviewing.

  1. dangerous code - code that could hurt the business or its users, for example by jeopardising application or data security

  2. bad code - code that has bugs or is prone to bugs, or where one cannot easily discern how it works and therefore whether it might have or be prone to bugs

  3. strange code - code that is esoteric or that does not follow best-practice or established patterns or guidelines

It goes without saying that I don't use these category names in my comments. I'm not telling people "this is bad" or "this is dangerous". It's important to phrase things positively. I have been on the receiving end of reviews where the reviewer has said "this is bad," and it stings. On the one hand, they were right, on the other, they were tactless. We should always try and lift each other up.

There are two problems with code that fits into these categories.

First, that once it is merged and lives in the codebase, anyone who reads it who doesn't know any better (a junior, or a new hire) will assume that is the standard of code which is permitted.

Second, when bugs occur in code, the issues they cause are rarely isolated to the source. What I mean by that is, usually when we write code, we assume that the code before it ("upstream") is correct to some reasonable extent. So when that code isn't correct, and it pushes some bogus data downstream or throws an error, that causes a cascade of issues.

It is the reviewer's job to put proposed code changes in the context of the wider system and scrutinise them from a bird's-eye view. When reviewing, it is useful to work backwards: look at code from the outside in.

Code style? What's that?

Following from the last section and its mention of strange code, style is really the least important part of code review. In fact, many programming language toolchains come with formatting utilities and the like to enforce code style from the get go. Go, for example, has go fmt. And Prettier is now so common in JavaScript projects I more or less consider it part of the standard library.

Note that by code style I mean specifically the aesthetic appearance of code. Not implementation or architecture patterns that are established in a codebase.

Code style is one of those cases where the "disagree and commit"4 principle really comes into its own. A team should decide on a code style once early on, set up the relevant tools to enforce it, and ensure that during onboarding of new members their editor is able to apply the chosen code style automatically. I rarely think about code style these days.

In the first year of my career, I was very concerned with style. I would point out when a semicolon was missing, refactor functions to be arrow functions (because "they are more elegant", and were new and shiny at the time), argue till dawn that spaces are better than tabs, reorganise imports to be ordered alphabetically, and so on and so forth. Basically, I was a pain in the ass.

The fact is, most code style concerns are irrelevant in the broader scheme of things. Don't get caught up in the weeds of code style, as once you start pulling it's hard to stop, and you see them all over the place. Save yourself the time, focus on higher impact concerns.

Large code changes are bottlenecks

Or, how to lose friends and alienate developers.

I am very guilty of submitting large PRs. It's something I am working to improve. It came up in my most recent six month review. I will be working on one thing and see a problem with something else related, so I start fixing the other thing too. Before I know it my code changes amount to 50+ file changes. (Sorry Harry.)

Code changes of this size are very difficult to review. They also pose a risk when you consider that usually changes in a single PR are deployed together, so it can be hard to identify what parts of the changes caused any new issues since the deploy, should they arise.

Stay focused on the task at hand. I have started keeping a list of parts of the codebase that I would like to revisit when I have the time. I try not to look at the list too much, but I know it's there. I find compartmentalising in this way useful because it means that I am not skipping over things that need attention, or putting the blinkers on. When I find a task that touches code on that list I take it as an opportunity to tick it off.

Code reviews come first

When I get a request to review code, I almost always stop what I'm doing to look at the changes immediately. There are many articles and thought pieces that address context-switching as damaging to productivity.5 For me, this just isn't the case. If anything I find taking breaks whilst working on one thing to look at another more conducive to my productivity.

Sometimes I jot down a few notes about where I need to pick the other thing back up, but most of the time, as the time it takes to review is often short, this is not necessary. The majority of PRs I review are small, the number of changed files being in the single digits.

Arguments for or against context-switching aside, in most instances code reviews should take precedence over whatever else you're doing. Waiting for a code review is painful. If you've spent hours, days, even weeks working on something, it's disappointing when that momentum slows if a code review is not given in a timely fashion. When you're a member of a team, your responsibility is to the team.

On any given day I may spend between one third and one half of my time reviewing code. This means that my peers are not left waiting, are not blocked, and can get started on any feedback quickly.

Synchronous code reviews

Due to the way we work as developers, using tools like git, GitHub, and Gitlab, Slack, or whatever, that facilitate asynchronous back-and-forth, it is very common for someone to leave a review and then disappear when the author has resolved their feedback. Or for the code review process to be dragged out because replies drip through over the course of the working day/week.

This is true especially for remote-first teams, of which I am a member. I work remote 100% of the time. I have visited my employer's office once (two consecutive days) in over eighteen months.

A synchronous code review is a review where two or more people involved in the review process take a call together, in the case of remote working, or move to a desk or meeting room, in the case of office working. Lots of the time code reviews can be sped up dramatically if the author and the reviewer interact in this way. The author walks the reviewer through their work. The reviewer responds in real time. Ideas and feedback can be shared quickly. You can even go so far as to address any feedback immediately by pair programming.

If you find yourself spending a lot of time typing up comments during a review, consider involving the author in a synchronous code review.


References:

  1. https://en.wikipedia.org/wiki/Dogfight

  2. https://sdgluck.bearblog.dev/learning-to-see-positive/

  3. https://www.atlassian.com/agile/software-development/code-reviews

  4. https://en.wikipedia.org/wiki/Disagree_and_commit

  5. https://blog.rescuetime.com/context-switching/

#agile #teams #remote-working #code-review

- 5 toasts