Welcome to the second part of my article about code review. If you have not done so yet, I encourage you to first read the first part of this series, where I introduce the concept of code review and give some tips about checking the code that has been committed.
This second part will be focusing on what follows the code checking, and that is the writing of the review itself. So, without further ado, let us dive into that.
Principles for Review Writing
Once you have reviewed the code, comes the time of writing your feedback to the submitter. As mentioned in the introduction, this can be quite easy if all looks good and can be merged as-is. But if not, you enter the uncomfortable zone of having to explain to the developer what is not good (or even really bad) and needs to be changed or rewritten.
If you have to remember only one thing about this part, it should be the following: Don't make the developer regret that they submitted their code.
And in order to achieve that, to get my review as well perceived as possible, here are some considerations that I keep in mind while writing my feedback. Like for the code checking part, all might not apply in all situations, but it can make an useful checklist.
We play on the same team
To me, this is the most important aspect to keep in mind while writing, because it helps set the global tone of the review. Even if it can be difficult sometimes, the main point to remember is that in the end all the people involved are in the same team and work towards the same goal: deliver good work and improve the global project code quality, which benefits to everybody on the team.
So, the goal here is absolutely not to show off your skills or make the submitter feel stupid, to the contrary. My view is that the goal should be to guide them towards the improvement of the code they submitted, to make it acceptable to merge. If you are reviewing code from a junior developer, this might actually be part of the coaching process.
At one of my previous jobs, some of the developers on my team were literally afraid to submit a PR because they knew that the technical lead would just take joy in tearing their code down. I personally do not see the benefit of that for anybody. It creates anxiety and can even be counter-productive because, if the developers are 100% sure they will get teared down no matter what they produce, they might end up being tempted to not make any effort at all anymore.
To the contrary, I sincerely believe that writing with humility and using appropriate language and formulations are key aspects of writing a successful and well perceived code review. I always keep in mind that that person is a colleague I am in contact with every day or a friendly OSS contributor who allocated some of their free time for us.
On another level of the team consideration, I would also like to mention that I think it is preferable that code reviewing is done by different developers of the team when possible. This will avoid getting into a situation of having "THE reviewer against the rest of the developers”. It will also help spread the global code base knowledge among the team, as I mentioned earlier. And, most importantly, there should at least be a second reviewer to validate your own PR's, shouldn't there 😉?
Of course, this does not prevent having someone like the technical lead who gets an eye on all PR's to keep a real global overview of the code base, but that person should ideally not be the only one to actually perform the detailed code reviews.
Code Review is not Code Rewrite
Another important fact that I keep in mind, is that I am reviewing someone else's code, not rewriting it. The idea is not that, at the end of the process, the code is changed in a way that it looks as if it was my own.
What I need to check is whether the code fulfills the necessary quality to be merged into the main branch of the code base, and if not, to let the developer know what are the minimal changes needed to get there. This might of course imply following some guidelines or maintaining some level of consistency and structure with the rest of the code base, but not mimicking my way of coding.
In the same spirit of not rewriting, I would recommend to avoid amending the code yourself. I see a few reasons for that:
- By making the change themselves, the developers will gain knowledge and experience, and might also better remember how to proceed when facing the same situation or problem on a later occasion.
- It avoids the risk of having an "intrusion" feeling perceived by the submitter.
- Last but certainly not least, it saves yourself some precious time!
As far as I am concerned I never amend PR’s at work, except for example if the person is on holidays and we really need to merge the code. I almost never do it on the Umbraco community PR’s either, except in situations where the change is really small and the developer does not react for a few days, or for adding the French translations of newly added labels 😊
If I do end up amending the code, I always make sure to leave a comment behind explaining why I did the change myself and saying that I hope it is not a problem. It does not take much, just a few words, to easily avoid misinterpretations or misunderstandings about what I did. And if the person answers that it is in fact a problem for them, then I will know it for next time. Up to now, however, I never received negative feedback.
Besides leaving a comment, I then also evaluate the opportunity to leave my name (which is added automatically by Github in the PR comments for example) in the PR's credits. In most cases, I suppress it, because I do not want the submitter to think that I made some amendment just to get my name in there.
Amending the code
Have a look at the authors' reactions 😉
Write a review you would like to receive
An easy way to check if you are on the right track in your review writing, is to ask yourself how you would react if you were the one receiving that feedback. This will give you a direct indication of how your review will be perceived by the submitter.
Besides what I already mentioned earlier, writing well perceived reviews can have direct benefits for you as well. Keep in mind that the person you are reviewing today, might be the one reviewing your work tomorrow, and will probably be more inclined to be nice to you if you were nice to them.
This is also a crucial element in OSS reviews in my opinion, because the developer is mostly doing all this during their free time, and we do not want to discourage them from coming back. To the contrary, we want returning contributors! 😊
Personally, when writing my reviews, I focus on the following elements:
- The developer did their best at this point, otherwise they would not have submitted their code. So, if it does not look good, I explain how to improve it rather than criticize it.
- Nobody likes their code to be turned down, so I aim at making constructive remarks, giving hints or directions to go further. I also pay attention at staying friendly and polite, even if I had a bad day at work.
- I always try to make suggestions rather than request or impose actions, and I formulate my remarks as much as possible as questions or hints: “Wouldn’t it be better if”, “Could we do …”, “I am wondering if …”, “I think we can…”, etc.
Although in most cases I know the answers and outcomes of those interrogations, by doing so, the developer will probably feel more comfortable and involved in the review process and in the decision to ultimately modify their code, as they are not just imposed to change things around. They rather get invited into a thinking process to evaluate if what I am suggesting might indeed be better or not. My feeling is that this opens an opportunity for discussion or argumentation rather than giving the feeling of a closed discussion.
The other aspect of this, is that I might often be right but, as I mentioned earlier, I might also not have the complete view of the functional requirements, and maybe there is a very good reason for implementing things in the way it was done. Or, maybe, I did not know about this new language feature that makes the code much more efficient.
So, besides making my remarks easier to accept, this approach of presenting them as questions can also help me look less "stupid" or arrogant in the case that I was actually wrong. Because I can then just close the discussion naturally with a comment like “Ah ok, right, I did not know/think about that. Well done!”
A last important tip I would like to mention to close this chapter, is to stay positive and encourage whenever possible, to show enthusiasm if something is good. Code reviews are like many things in life: we often focus on what is causing problems and do not mention what is good. Everybody likes to receive some kudo's for a job well done, so I try to pay attention to that.
And I cannot resist ending this part by showing you the following Tweet, which appeared on my timeline a few days ago, and which sums it up pretty well, I think.
Adapt the writing to the submitter
This tip is kind of two sided. For the people you know well, I would recommend to adapt your writing to them. Once you know a little bit of the personality of your colleagues, or returning OSS contributors, you can adapt your writing in a way that you know will better fit their personality: be direct or put a nice paper wrap around your remarks, use some humour or irony, or just stay plain serious etc.
Below is an example of how I ended up asking my colleague Rudy to remove client-side debug logging that he regularly forgot to take out:
Why spend time writing long sentences if you can just put a word or two, right 🙂?
But if you are writing a review for someone you know little, or maybe not at all, I would recommend the exact opposite: play it safe and stay as neutral as possible. Consider the possibility that English might not be the submitter's mother tongue, or the fact that they might have other cultural references than yours. Also, you do not know their personality at all. In such situations, my focus would be on clarity and I would avoid things like humour, irony or practical jokes in order to prevent any misinterpretation.
Things can go wrong
Everything I have been telling you up to here aims at helping you write a code review that is eventually received in a positive way by the submitter, who will in turn be ready to do the changes you have asked them to do.
But despite all the good efforts that you will put into the process, in the end, as always in the software world, sometimes things will not go as expected. It is not pleasant when it happens of course, but it is part of the game and therefore it can be helpful to have some hints on how to react in such situations as well, because those are the least comfortable to handle and get passed.
The submitter does not play their part
These are situations where, for example, the submitter does not show any goodwill in making the adaptations you have asked for (so I am not talking about constructive discussions about why they would not want to do the change), or where the submitter makes recurring mistakes from pull request to pull request, despite you making them the remark every time.
Fortunately for me, I did not encounter such persons too much, but I remember one from a few years ago who, among many other things, kept performing "empty" string checks with string.IsNullOrEmpty, while I kept telling him he should use string.IsNullOrWhiteSpace in those scenario's. And even if he made other amendments to the PR, he would regularly leave those string.IsNullOrEmpty unchanged.
This got quite frustrating and irritating through time, because I felt that he did not pay attention or did not (want to) learn from the reviews I wrote, which made me feel like I was losing some time that I could spend much better.
One thing you can do when faced with such situations, is ask for a second opinion, when possible, just to make sure that you are not exaggerating the problem for some reason. But if not, my opinion is that it is then quite all right to let some friendliness behind and be more firm, direct or insisting to get the changes done.
Because, in the end, you are the reviewer and if you feel like something really has to change as you asked, well, then the changes have to be made. It is always better to get things done through discussion and positive attitudes, but if you need to end up doing that last change yourself to put an end to the discussion and move forward, it is unfortunate but it might be the only solution. In the situation I just described, sometimes, after asking 2-3 times, I would just let go and update it myself in a next PR. To be honest, I think that if you get that far, there is probably a more global problem with that person than just code reviewing, but that is another story.
The reviewer is not available
This one is probably more linked to the global team organization than to the code reviewing itself, but I felt it was interesting to mention as well.
One question that I was once asked was the following: "How do you handle delays when people are too busy to review your code? Especially if they're the main person you really need to review it?"
If the reviewer is not available, I see two scenarios: the easiest one, where you can wait for their return. Then you just wait for it 🙂 The second one is when you cannot wait. Then you can either merge things yourself or create your next branches from your PR branch. In that case however, ultimately, the reviewer will come back and check your PR, so you might of course need to synchronize your subsequent branches later on with any changes you might make to your PR branch after the review.
Reverting the merge
This is of course the worst scenario that can happen as outcome of all this process, but even after all the careful reviewing and writing, and maybe changing, the merged code can have unforeseen effects once running in test (hopefully) or in production (argh!), and sometimes the most appropriate solution is to revert the code.
As I said at the beginning of this chapter, failure is part or the software programming world and we should all be ready to accept it and take action in the least unpleasant way. But since we have been talking about ways to avoid getting there, let us not make too much a case out of it and hope for the best 😀
Now go ahead and try
So, it looks like we have considered the whole picture now! My goal was to give you insights and maybe a better understanding of what kind of work and reasonings can all be involved behind the code review process. We have seen what a code review was and the purposes of it, I have shared with you the main principles and reasonings that I follow while reviewing code and when writing my feedback on it. I also emphasized some of the differences that I see between reviewing Open Source Software and Software At Work, and I ended up giving some advice on how to handle failing situations when they occur.
I really hope that the code reviewers among you will have taken away some new tips or new points of views to try out on your next code reviews. And if you actually do try some of these out, I would be very happy to hear your feedback about using them.
If you’re not a code reviewer but you have been code reviewed before, I hope that this article has highlighted the fact that we, code reviewers, do not write remarks just for the fun of it or to make your life miserable, but that the ultimate goal is to make it all better on the long term for everybody.
With this in mind, I would like to conclude with a last take-away that I think summarizes best my perception of what the finality of code reviewing should be. I call it the WWW principle (we work in the web after all), aka the Win-Win-Win principle:
- Win for the project that gets better code quality and a somewhat more homogeneous code base
- Win for the submitter who can improve their coding skills and knowledge by learning or thinking from the constructive remarks received
- Win for the code reviewer who can get a better overview of the overall project codebase and who can also learn new things from the code reading
If you think about it, all the elements that I detailed in this article series can be inferred from this principle.
I guess now the only thing left for you to do, is go ahead and try it out, and let me know how it goes 🙂
Resources & references
- More info about the Umbraco Core Collaborators Team that reviews community contributions to the Umbraco CMS.
- If this article has convinced you that we are a friendly bunch, and that no matter how small or how complex your contribution might be, it will be treated with enthusiasm and positivity, start contributing to Umbraco-CMS. Have a look at the issue tracker if you need some ideas!
- This article follows a presentation that I performed at the Codegarden 21 conference back in June, there is a video available.