Reviewing someone else's code can look quite simple at first sight. It can indeed be quite exciting and satisfactory when everything goes well: you basically go through the code, validate and merge it, say "nice job, thank you" to the submitter and move on to the next pull request. But when all things do not go according to plan, it can become a somewhat daunting or even frustrating experience.
"How do I tell the developer that their code is not that good? Or not good at all?" "How far should I go in asking to rework on some parts?" "Should I stay calm after mentioning the same issue for the 10th time to the same developer, or should I start expressing some level of irritation?"
These are the kind of interrogations, among many, that people performing code review will be facing at one point or another. I certainly have, on numerous occasions, and they are not always trivial or easy to deal with.
I have been reviewing code for quite some time now and I have checked several hundreds of pull requests, as technical lead or coach at different clients throughout the years, and also as member, for a little more than one year now, of the Umbraco Core Collaborators Team, whose main purpose is to review the pull requests submitted by Umbraco Community members, and eventually merge them into the Umbraco repository.
And if giving the appropriate feedback can already be a delicate matter when reviewing code from a fellow colleague, what I realized since I joined the Collaborators Team is that it gets even more delicate and significant when it comes to Open Source Software (aka OSS) because, in that case, I mostly interact with people that I know little, or even not at all, and who probably coded gracefully in their spare time to support the project.
What I propose to share with you in this two-parts article are some of my experiences and tips in reviewing code, both at work and on OSS projects (with Umbraco as source of experience for me). More specifically, I would like to explain the different approaches that I follow for my reviews, and also highlight how, in certain situations, I consider work and OSS reviews with slightly different perspectives.
My goal is that, if you are a code reviewer, you will come away with some tips to apply on your next review. And, if you have ever been "code reviewed", I hope that you will get a better insight at what probably happened behind the comments and feedback you received.
In this first article, I will focus on some general considerations about code reviewing and then on the code checking part of the review. The second article, which will normally be published in issue 80 next month, will then focus more on the actual writing of the review.
Code Review: What?
Let us start by having a common understanding of what we are going to talk about.
As an "academic" definition, I would say that code reviewing is about evaluating someone else’s code, with the purpose of verifying whether or not that code is eligible to get into the project actual codebase. And, if not, to communicate to the submitter what needs to be done to become eligible.
Or, if you prefer the shorter and probably more usable version: checking that the code can be merged or else give feedback.
The interpretation of evaluating, or checking might depend on the time that can be allocated to the code reviewing activities or on the goal to achieve: quick read through or detailed reading, actual testing of the code, etc.
A point worth noticing is that both definitions contain the two distinct parts that I see in the code review process, and which I think are important to distinguish: a technical part of course, since I am reading and evaluating code, but also a communication part, which I believe is evenly important, if not more.
Indeed, in case some rework is needed, my feedback needs to be clear and incentive, so that the submitter can easily understand what I mean and what I expect, and also, most important, so that the submitter will be willing to do the actual changes. After all, this is the goal of the whole process, isn't it?
Code Review: Why?
Defining the purpose of the code review process is a necessary initial step in my opinion, because reviewing code is a time-consuming matter. It will take time away from the development activities, and therefore cost money and have an impact on the timeframe of the deliveries, so it must be performed with clear objectives.
There may be many of them, depending on the project size and also on the time that can be allocated to the code review activity for example. Here are a few common ones:
- Make sure that the code is following predefined guidelines or best practices within the project or the company.
- Verify, and therefore increase, the quality of the code before it gets merged, deployed or sent to the testers (e.g. detect and remove obvious flaws, copy/paste errors, etc.)
- Maintain a sufficient consistency in the code produced by the different members of the team. An important note here: it does not need to look as if all the code had been written by the same person, but some degree of homogeneity is always welcome and necessary. This point might of course already be covered by predefined guidelines, if any.
- Share functional knowledge or awareness within the team, because people will get a look into parts of the code that they did not write, and by reviewing them, they will at least know those parts exist and have a basic knowledge if they end up working on those later on.
- It can be part of the coaching process of more junior developers.
Code Review: How?
So, now that we have a common definition of what a code review is and that we identified its purposes, let us see how we can actually achieve this work!
I guess the first tip that I can give you, is to keep in mind that reviewing someone else's code is a totally different activity than reviewing your own code.
To start with, the first step of understanding the submitted code can sometimes be a challenge by itself already, while I do hope that you understand your own code when reading it 😉 This might be more challenging in OSS because you might be facing parts of the project that you have really never touched before, and therefore you might need a bit of digging-in to understand things globally, before actually being able to start the review per se.
It has certainly been the case for me, more than once, when picking some of the Community Pull Requests (aka PRs). Things like automated testing with the cypress library for example, were totally unknown to me up to then. Or only recently did I review a PR that was applying database migrations.
Then, once that code reading step is finished, how do you go from there and express your remarks and questions in a clear and friendly manner? I mean, when verifying and testing your own code, if you find something wrong, you just change it without the need of giving any explanation. And if you are not happy with your work, you are free to call yourself whatever bird's name you wish. I would recommend not trying this on someone else...
In other words, I truly believe that you need to handle code reviewing with a different state of mind than when checking your own code. Easily said, but how do I do that, you might ask? How do I get in that "code review state of mind"? Well, I guess the time has now come to get to the concrete stuff and share with you some thoughts and principles that I try to keep in mind during my code reviews. Take a seat and let's get started!
Mike's Principles of Code Review ™️
As mentioned in the introduction, I will split them in two main categories, the ones that are useful when reading and checking the code and the ones that can be used at the moment of writing the review, which I will cover in next month's issue.
They might not all be applicable to all situations, depending on the project scope or budget, but let us say that they can make a useful list to keep at the back of your mind and pick from as appropriate.
Principles for Code Checking
Code checking is of course about reading and validating the code, but there are many ways to achieve that. Here are some ideas and hints that I think can have a significant impact on how efficiently this activity is done.
Get into a constructive mood
First of all, I see code review as the achievement of a process, whose outcome is that new or better functionality will be added into the codebase. Quite a joyful insight, is it not?
So, my goal when checking the code is to be able to approve and merge it while asking minimal extra effort, ideally none, from the submitter. My plan is not to go and hunt the last bit that might be made better.
Of course, it does not mean that I just take a quick glance and approve the code just because the indentation looks good but, basically, if the new code makes sense, follows guidelines (if any) and integrates smoothly with the existing code, I’m pretty much good to merge it and pick my next PR 😊
This aspect is especially important on OSS code reviews in my opinion, because people give their time for free, and therefore, in return, I think it is fair to ask them as minimal changes as necessary, give them positive feedback as much as possible and thank them for their work.
Test the changes locally
If time or infrastructure permits it, I test locally. Testing locally helps verify that the code still compiles (you'd be surprised) and that the update behaves as expected. It can also help to spot flaws that escaped compile time.
A good illustration of this, are the Umbraco translations files for the backoffice. These are XML files that are loaded at start time and then parsed to retrieve the translations, based on a key-value logic. Those files will typically not be handled at compile time, so let's imagine an update to a translation value in one of those files that would break the XML formatting and somehow be unnoticed by the developer. If not tested locally prior to merging, you might end up having the backoffice not usable anymore in that specific language because an exception would be raised each time a translation would be looked for, which is on every page of course.
Let's keep it between us, but I actually once crashed the French translation file (all those quotes and accents 😉), but since I tested locally, it fortunately did not make it to my PR or to the code base. So, I'm really talking from experience here...
At my current work, testing locally is not always easy to do, not for all part of the software at least, so we have set up a dedicated integration environment where the merged code is deployed automatically. This means we can test right after merge and act directly if something is wrong, prior to sending things to the testers.
But for the Core Collaborators team and Umbraco-CMS, I always get the PR and run it locally to verify that everything works as expected. I find this step quite necessary because, in the end, a lot of PR's will get merged, both from contributors and from the Umbraco development team, and eventually form the next Umbraco release. We need to facilitate that complex merge process and try to eliminate up front all the errors we can.
Get an appropriate pull request
What I would like to emphasize with this point, is the fact that I try to handle a pull request that is adequate to the circumstances in which I will be reviewing it.
For example, I always try to handle a PR in one go, so I will not pick a complex one if it is 15 minutes before the end of my day. The chance is indeed big that I will not be finished today and that I will spend the same 15 minutes tomorrow morning to get back into where I was.
Following a similar reasoning, I will more likely pick a big, complex PR at the beginning of the day, when my brain is still fresh, and leave the smaller, more trivial ones for later when it will not really matter that my brain is tired after a day of meetings and new developments.
When you look at the pull requests in the Umbraco repository, it is in fact nice and useful to have such a diversity of PR's, ranging from full new features impacting 20 or 30 files to very small (and sometimes very useful as well) updates of one or two lines in one file. It means that I can pick up whichever one fits my mood or energy level of the moment 🙂
This is usually much less applicable at work, because the pull requests are much less numerous and I am lucky when I have 2 or 3 of them to choose from.
Breakfast PR
Dinner PR
Code Review is not Functional Review
I feel this is also something important to be aware of and to keep in mind: at this point, my job is to make sure that the code is all right on itself, not that it fulfills all the functional requirements. That is the task of the developer in the first place, unit tests to some extent and then, in bigger teams, to the testers after I have merged and deployed the changes. If the developer missed one of the requirements, I consider it is not my task to spot it in this process.
Of course, since I know (some) of the functionalities, and since the title and description of the PR will give me indications of what has been achieved, if something looks illogical, or if I know the code is wrong or not covering all cases, I can mention it or ask a question about it so that the developer can make a double check. There is no point in letting errors go through on purpose, but the main idea is that I do not have to read the functional requirements in order to make my review.
This point is probably a little less applicable in the case of Umbraco because we usually have a description of what has been changed and why. Either through reference to an issue in the issue tracker or right in the PR submission. In that case of course I need to check that the update solves the issue mentioned or fulfills what has been promised.
But I am working alone on the project
I once got the question whether code review was interesting in small teams, or small projects, and my answer was: Yes, maybe not to the same extent as for big projects with bigger teams and budget, but it can always be interesting for the reasons I mentioned earlier.
I thought about that question again later on, and my answer could actually have gone further, because I think that code reviewing can in fact be interesting even if you are working on your own on a project.
If you are a developer and have never done it before, I truly encourage you to try at least once to play the code review game on yourself. Not the writing part of course, but before committing your code and creating that new pull request, go through all the changes you are about to push and merge.
Because you will be checking these as a "recap" outside of the coding process itself, you will get a high(er)-level, more global view of your changes. And be it inconsistencies in the way you handled a similar problem, be it some possible refactoring to generalize a functionality, or just commented out code to remove or debug logging that you forgot to take out, you might be surprised at what you could end up detecting in your own code.
And even when working in a team, this can be a useful exercise. I regularly review pull requests where it is clear that this was not done, and then I have to start writing remarks like "remove commented code". So I really encourage you to make that extra effort. I always do it for my own code changes, and it actually does not take that much of extra time. Detect the potential obvious flaws yourself, it will make your PR look better and spare some time of the reviewer 🙂
To Be Continued...
It is on that last piece of advice that I will end this first part about checking the code; in the issue next month we will get our hands on the writing part of the review, which can be quite challenging too. I hope you have enjoyed it so far, and see you next month!