If you’re new to the code review process it can be pretty daunting, not only are you putting your code on display for others to judge, but you’re actively soliciting feedback on it.

But don’t worry, they are a great tool to help you to learn and the best way – in my opinion – to learn a new codebase is to get others to review your work. A good review will highlight deviations from the defined standards, and potential issues, maybe you rolled your own function to do something, but it turns out there was already something that did it for you that wasn’t surfaced well.

I’m mostly going to be talking about standard, asynchronous code reviews, but these points also stand for doing Over-the-shoulder reviews as well.

Some ground rules

Before we start doing any reviews, let’s set rules to follow. Don’t worry they are pretty simple.

As a reviewee

1. The code should be complete and ready to review.
   • Don’t put half-complete features into a code review, you aren’t going to be committing that code so having an approved review would be pointless. The process would have to start again when you finalize the codebase.¹
   • Don’t create multiple commits for a single feature to be functional. They should be atomic units of work that can be rolled back without breaking the entire codebase.
2. Your commit message should be informative
   • I recommend commit messages having a standard in general, but they must be detailed enough so that anyone scanning commits can figure out the intent of the change and ideally the scope, they should also link to the ticket you are working on (assuming you are using task tracking software)
3. Assume good intent
   • Having your code reviewed by others can feel personal, but it’s a helpful process that you will learn from. Assume the good intentions of anyone reviewing your code and that they have good intentions for you AND the project.
4. Be prompt with responding to feedback AND addressing any issues flagged
   • This doesn’t mean refreshing the page every second, but it means when you get an email telling you the review has a comment, respond to it promptly, and maybe grab the reviewer into a call if you need to talk over some details. Don’t forget to leave a follow-up comment for posterity.
5. Assign relevant reviewers
   • Don’t assign it to your buddy because they will wave the review through, assign it to someone who knows that area of the codebase and will be able to vouch for the quality of the code. If you don’t know who that is – talk to your lead and get them to assign someone on their behalf.
   • In the same vein, don’t assign the entire engineering team as a reviewer, try and limit it to a max of three people.

¹In rare cases you might need to have a half-implemented feature in the codebase, but it should be completely sectioned off behind some feature flags and clearly labeled as incomplete, along with tickets in your task tracking linked in comments. Ideally, the code never touches the repo.

As a reviewer

1. Remember the Reviewee is a person too
   • I get it, you go through a couple of hundred reviews in a day and see the same issue repeated by different people. The reviewee is trusting you to help them learn and that’s a lot easier if you aren’t talking down to them.
2. You are on the hook for the code
   • If you are a reviewer on a piece of code, which then goes on to break the codebase, both you and the reviewee will be spoken to by the lead engineer and tasked with fixing it. Don’t phone it in with the review.
3. Leave constructive comments
   • “Why would you write it like this??” isn’t helping anyone. “I suggest rewriting it like X, because reason X, Y, and Z” is far more constructive, the Reviewee will learn something and you’ll have the same outcome you’re after.
4. Point out the good
   • See something that you really like or a smart piece of code? Call it out! Code reviews aren’t just for pointing out problems.

Okay, now that’s all out of the way, let’s get onto making and doing good reviews!

Intent

Every time you are submitting anything to the codebase you have some kind of intent, when you are working in a team you want to be crystal clear on what that intent is because people can’t read minds, yet.

We first need to make sure that the commit message clearly defines what the code is intending to do. For example:

XXX-000 - Implemented backflip animation, tuned to take X seconds. 

Starting off with the ticket number will help your reviewer find that ticket and confirm that the work actually solves exactly that. The message afterward then further defines what you intended your code to do. This will also help people go over daily commits quickly go over things that have been committed.

Sometimes we would have bugs appear in the codebase that I could easily pin down to a certain day with our QA, then I’d gather a list of all commits in that time range and go over their commit messages. 9 times out of 10 I could find the offending commit from their message alone, then find the line of code that caused the break.

The commit messages can also be used to write up patch notes, so keep them to the point.

Doing the actual review

Does the solution “make sense”

The code needs to solve the problem defined but in a sensible way. Let’s go over an example.

We want the balloon to sway back and forth.

When we see the code for the review, which claims to solve this task we find the following:

using UnityEngine;

public class Balloon : MonoBehaviour
{
    void Update()
    {
        var World = GameObject.Find("World");
        World.transform.position += new Vector3(Mathf.Sin(Time.deltaTime), 0, 0);
    }
}

Not only is the reviewee moving the entire world instead of the individual balloon but they also “hard coded” the amount of sway into the world.

This solution clearly doesn’t make sense, so now would be a good time to have a conversation with the reviewee, talk over their solution and walk them through the problems with moving everything instead of just the object itself. They learn from it and the code base doesn’t get that submitted into it.

Can you spot some problems?

When you are reviewing the code, keep an eye out for things that can cause nasty bugs. My personal pet peeve was memory leaks/memory stomping bugs. They can be difficult to spot in a review but they can cause even harder-to-diagnose problems in the future.

Looks innocuous right?

GameplayCueDataMap.Add(ThisGameplayCueTag) = GameplayCueDataMap.FindChecked(Parent);

https://www.unrealengine.com/en-US/blog/memory-corruption-finding-and-fixing-elusive-crashes

To a later point, you can use a review that points out some non-obvious problems as an example for others to learn from. The intent is never to shame the reviewee (you could even make a new review so you avoid naming the original engineer) in that case, but as a learning moment for everyone.

Is there debugging code still in the review?

I’m not talking about intentional debugging logs.

void SomeFunction() {
    UE_LOG(LogTemp, Error, TEXT("Oh its working!"));
}

Make sure things like this don’t get committed. With huge amounts of code, I can understand how that can be mistakenly left in, just remove them.

Data files? Are they in the review? Are they set up correctly?

This might be more of an Unreal Engine practice, but we used to take screenshots of associated blueprints and marked them up with “what changed” as our code review tool couldn’t display their binary assets.

Make sure they are included in the review if they are needed to make things work.

Does it fit the standards?

This is usually down to style, but keep it consistent otherwise it becomes difficult to read everyone’s unique style.

I prefer private variables to be prefixed by m_SomePrivateVariable but I’ve also seen:

m_SomePrivateVariable
pSomePrivateVariable
somePrivateVariable
_somePrivateVariable

Is it the end of the world? No, but it should be addressed to keep everything looking clean and consistent. Speaking of which if you need a coding standard for Unreal, I got you.

Avoid flagging the same issue multiple times in a review, for instance, if someone named multiple variables with the same mistaken camel case then just write a comment letting the reviewee know that the case is incorrect throughout.

Did you find a great review to learn from? Share it

Share it with others in your team. I remember one code review in an entire dynamic content stripping system which ended up having ~150 issues flagged in it, from the back and forth I had with my reviewer I was able to find a simpler solution that also was more performant (Simon if you read this, you rock dude).

I used that review as a form of documentation on the system when I was handing it over, the comments raised and the conversations about the system raised within it helped explain the intents behind the overall system and also some of the intricacies

Review around the code change

Suggested by Jameson Thatcher

Code rarely exists in a vacuum, if you are reviewing a piece of code that other functions previously made use of then make sure to check their still functioning as you’d expect. A simple change may end up changing the intent of other functions down the line.

There may also be helper functions that already do similar things which are more appropriate than writing it again.

Pull down the change

This might go without saying, but I’ll say it anyway, make sure you actually test the code change at the very least compiles locally when you pull down the change in the case of a pre-commit. You’d be surprised how often you’ll find code fails to compile in a rapidly evolving project when you are dealing with shelved code.

Want more?

MIT wrote a good post on best practices for code in general here, which is a pretty good read.

Wrapping it all up

Hopefully, that’ll help you get through your first review as a reviewer or a reviewee. Remember the ground rules and have a great time learning from each other.