Why even have code reviews?

Code reviews are a chance for you to get feedback from your team on changes you intend to introduce to the codebase. Your reviewers may see things you missed or point out potential problems you didn’t think of. Here’s the thing many developers forget about code reviews: you want your reviewers to identify problems with your code. You want to make sure your contribution to the codebase is of good quality so that you’re not the source of bugs or code smells that are going to cause problems down the road.

Do you like putting your code up for review? Many developers I’ve worked with tell me they don’t. But if criticism is something developers are supposed to welcome, why the negative attitude? Well, my guess is they’ve probably gotten a lot of crummy feedback from cruddy reviewers. I’ve been there, on both sides. I want to share a few ideas and bits of advice I’ve put together from experience. What I’ve found is that the smoothness of a code review is not based entirely on the reviewers’ actions; the review requester can sometimes set themselves up for failure. My tips for both reviewers and review requesters are explained below.

As a reviewer

Remember that text is lame

Code reviews in my experience have been about dropping comments on someone’s code. Comments are text and text is unfortunately not a great means of communication for code reviews. The review requester on the other end may put their mind in a defensive mode as they brace for a volley of criticism. As soon as a comment pops up they may treat it as a potential threat, maybe even taking offense where none was intended. Needless to say this can blow up all too easily. You can try to mitigate this by maintaining a friendly and positive tone in your messages.

• Try not to direct criticism at the review requester themselves (whether intentional or not). Avoiding the use of “you” can go a long way. For example “you forgot to check for null” could be written as “we may want to check for null here” or “should we check for null here?”.
• Point out things that you actually like about the review requester’s code and drop a nice comment.
• If you think something should be changed, be clear about how it should be changed. Many people will (understandably) get upset when you just poo poo their code without offering constructive alternatives.

Even better: if you’re able to just get on a call with the review requester you may be better off talking about their code over phone/video.

Look for real problems

Let’s talk about coding style. In C# you may prefer to implement a property getter with the lambda syntax while someone else may prefer to implement it the traditional way. This can be a touchy subject for many. You can use code guideline documents and automated tools to try to keep all your developers coding the same way but these strategies give off a sort of authoritarian vibe that never really felt right for me.

Let’s put things into perspective. Your team is not painting Sistine chapel. Most likely you’re constructing a piece of machinery that’s not going to be seen by anyone other than yourself and a few other devs. Your boss doesn’t care about how “perfect” your code looks. Your product owner doesn’t care. The CEO doesn’t care either. The reality is that “perfect” code doesn’t actually help you or the business when you could have achieved the business’ goals with “good enough” code much faster. Let’s take a concrete look at the property getter example mentioned at the start of this section.

// Traditional getter
class MyClass
{
  public int MyGetterProperty { get { return 5; } }
}

// Lambda syntax getter
class MyClass
{
  public int MyGetterProperty => 5;
}

As a reviewer I wouldn’t complain about either of these styles, even if they’re used inconsistently in the same pull request because it’s clear to me that they’re equivalent. If I were to make a stink about not following some kind of style guide, I’d just be wasting my time by writing the comment, wasting the review requester’s time by making them fix it, and ultimately delaying the delivery of the code that works just fine as is. In other words it’s “good enough” as is. Yes some may argue that developers will get confused if they don’t all follow the same standards but I’d say those developers are better off learning new things and being free to experiment with new styles. They’re anyway going to have to look at code from other teams, other developers on the web, etc, at some point.

And that brings me to my core recommendation for this section. If you see code that’s “good enough” but hasn’t been typed into the file with the strictest adherence to your refined tastes, please remember that it doesn’t really matter. Keep that ball rolling by passing the code review (unless there are real problems outstanding of course). If you really need to, you can probably correct these small imperfections in your own pull request at a later date (maybe just sneak it in with whatever you’re currently working on).

Other examples of “good enough” code objectively not worth nit-picking:

• Typos in variable names. If you understand what was written, it’s “good enough”.
• Typos in comments. If you understand what was written, it’s “good enough”. Comments don’t even get compiled, man.
• Use of whitespace (unless it’s particularly egregious of course). If you can read it, it’s “good enough”.

Rate your criticism

Sometimes reviewers will just pile a ton of criticism onto the code leaving the review requester asking themselves “do I really need to change that?”. It can help a lot to classify your criticism into categories like LOW, MEDIUM and HIGH to show the review requester how important your criticism is. Below is a ranking strategy I like to use.

As a review requester

Be proactive

You just spent 4 days coding up your masterpiece and you’re eager to get that code merged so you put up a pull request… and your reviewers completely rip you to shreds in the code review ensuring your masterpiece never sees the light of day. When this happens it tends be with larger work items that get designed and developed in isolation. The reviewers may be seeing the implementation for the very first time and totally disagree with how it goes about solving the problem. I’ve even seen cases where the review requester completely misunderstood the work item and wasted days coding the wrong thing. You’re pretty much guaranteed to have a bad time in the code review if you tackle large work items in isolation because every one of your reviewers is going to have their own opinion about how the solution should work, and that opinion is likely to conflict with what they see in your code. You may spend a long time trying to address your reviewers comments and concerns delaying the delivery of your work which ultimately hurts the business. Alternatively, maybe due to time constraints, the team may be forced to just bite the bullet and let that heap of manure you coded into the codebase with a weak hope of “cleaning it up later”, which also hurts the business, albeit in the future.

Instead of putting yourself through the wood chipper over and over again you can take proactive measures to help get your code through the review more efficiently.

• It’s surprisingly easy to dodge the risk of doing something out of left field. Talk to your peers about your design before you get too deep into the code. I’ve found that even a simple 15 or 30 minute whiteboard session is often enough to iron out most of the design kinks code reviewers would otherwise get hung up on. Your reviewers will be comforted when they see what they’re expecting to see when you request that review.
• If you think your code may ruffle some feathers it may be a good idea to actively walk through the code with some of your reviewers. This gives you a chance to explain everything as you go and respond to criticism immediately. Also, people tend to be nicer in person than in writing.
• Invite other developers to work with you. When your peers have directly contributed to the code being reviewed they’re likely to understand it much more thoroughly. Also, as a bonus, you’ve got a buddy to fight alongside you if things do get a little heated.

Listen to good feedback

It can be hard to take criticism especially if you’ve worked hard on your code. You may feel the impulse to reflexively dismiss all criticism your reviewers throw at you because you’re sure your code is perfect. Thing is… it’s probably not. Don’t forget, you want your reviewers to find legitimate problems with your code – it’s why you invited them to review it in the first place right? A little humility can go a long way here. Be prepared to accept that your reviewers may make good points that you’ll want to address by changing your code.

Let the small things slide

Hopefully your reviewers are giving you good feedback but if you are getting some small nit-picky superficial comments, sometimes it’s just faster to take a minute to throw in those tiny changes even if you don’t fully agree with them or feel it’s a waste of time. It may save a ton of time you’d otherwise spend arguing over stuff that’s irrelevant. If it continues to be an issue however you may want to talk about it to get everyone on the same page.

Stay out of comment fights!

This goes for both review requesters and reviewers. You’re definitely going to disagree on things. When you do, it’s always best to resolve disagreements over a live call. Text is a poor means of communication for this sort of thing because people can easily misread the tone of the writer. All the social stuff aside, the raw mechanics of communicating with somebody synchronously (like over a call) is just way more efficient than two people typing and reading comments over the course of hours. It really doesn’t make sense to keep lobbing paragraphs of text back and forth when you could have resolved your issues within minutes over a simple call.