Unless you fly solo, you are most likely going to participate in a code review. Do you remember that question from your last software engineering interview: Not the "What is your greatest weakness?". We all know you are detail-oriented and that it is also your strength. How do you handle feedback? That is what I always ask during an interview, and it turns out, people handle feedback excellently. Their first code review is going to show if they were right about themselves. If used correctly, code reviews establish the feeling of being mutually responsible for the codebase and giving your absolute best to participate in growing it. Good reviewers will guide you and make sure you learn the dos and don'ts, the designs, and the approaches applied in the codebase. If used poorly, code reviews can discourage people from making changes, slow down progress, or even hinder their career progression. To give a good review, you have to be fully aware of what a code review is. Second, you need a process that you can apply over and over systematically across multiple reviews.
Code review is a quality assurance process where your peers review your code. Your peers called the reviewers while you were the author.
Being a quality assurance process, the code quality goals we want to achieve are:
Code review also serves many other (implicit) purposes:
Now you're thinking: "But Akos, what does this look like in practice?"? Am I doing something completely different? “. Well, it could be because you're not doing a code review.
First and foremost, code review is not human linting.
If you don't already have tools to do this for you, I would instead set up those tools rather than waste my time fixing such things. Code review is all about being specific and on point. Don't say something vague, such as "I don't like this" - tell me exactly what you don't like and why. It's also not about being smart and turning three lines of code into one. A 2015 study provided empirical evidence that code reviews bring the most benefit in terms of a project’s long-term robustness, maintainability, and evolvability.
Read more here
Giving better code reviews is all about systematically applying a format, an approach over and over across different PRs.
Let's take a look at how I'm doing it.
A good code review is a good mix of high-level comments about the current state of the code and lots of specific comments about parts of the code. Because of this, I tend to split my code reviews into two parts:
In this part, you should point out if the code doesn't solve the problem at hand if it introduces a vulnerability or compromises readability. Keep this part clear. You'll have the chance to elaborate on your points when leaving specific comments. If you're using GitHub, their interface supports this approach:
This way, when someone opens my review, they'll see some like this: "I like the direction where this is going, but please move the GraphQL mutations to the parent component!"
Instead of: "This import can be removed."
So they'll immediately have an idea if I'm OK with this change or not. Do they have to rewrite the whole thing or just address some concerns? In the end, your ultimate goal is to address every explicit and implicit goal we listed in "What is a code review exactly?".
I usually leave these types of comments:
I like to think of "change" and "question" as blocking comments and "FYI" and "nit" as non-blocking comments.
These are the most important comments that everyone will take dead-seriously so, please be specific here. If your only reasoning for a change is that you don't like it, that's probably not going to affect the long-term state of the project. Leave comments when you're absolutely sure that the current code might not be the best in terms of readability, it introduces a security flaw, or simply the solution applied here doesn't solve the problem at hand. Because this is a blocking comment, until it is resolved, the code can't be part of the codebase.
I ask a lot of questions; I’d say these are my most frequent types of comments. If you're not sure if a change is required, you can't go wrong with starting with a question. These are also blocking comments because they might become a Change, FYI, or nit.
The following two are non-blocking comments; you can leave the code as-is if you want.
You'll leave these types of comments typically for newcomers. On huge codebases, there might be different ways to achieve the same thing. The current approach is not inherently wrong; you just want to give a heads-up that there are other ways to do this. Give a slight hint to new people about the new pattern that you started practicing, the fact that now you can write tests in framework X instead of Y.
Leave the choice of change to them; they can fix it but don't have to.
Short for nitpicking. As the name implies, these things usually fall into the category of "What code review is not?". Different teams might label FYI changes as Nits; after all, nothing is set in stone.
Code reviews, when applied systematically, are a powerful tool in software development. If code review has a bad reputation in your organization, people probably are not using it correctly. What I saw most often is that they pointed out things that were either just personal preferences or didn't contribute to the quality of the code at all. But even if you're fully aware of what you should point out, there are dozens of different factors that influence how your review is taken.
The person whose code you're reviewing just started?
Is this person a seasoned professional?
These are all factors that you have to consider when leaving feedback after reviewing someone's code. The bottom line is, it's better to be safe than sorry.
It's better to start with a question that can lead to a meaningful discussion than being all smart about it and creating tension.
When I was 15, I sold my first piece of software. I started making simple websites about 20 years ago. Since 2011, I have been a professional software developer; I have worked in different fields such as FinTech, E-Commerce, Acoustics, and Workflow Automation.