Benevolent code review

Doing a code review is an exercise that seems fairly uncomplicated at first glance. After all, it's just reading code and pointing out anything we think isn't right, right?

But we often forget that very often we find ourselves on the other side of the mirror and that it is our code that is being audited. It is the fruit of our labor that is the center of attention of our peers.
And, depending on the level of benevolence of our colleagues, the exercise can suddenly become much less comfortable!

The word is out (well, you already had it in the title of the article!): we are of course going to talk about benevolence, mainly in the field of code review.
Why mainly? Quite simply because in my opinion benevolence should be the basis of any interaction with our colleagues on a daily basis, regardless of the context!

Let's start by listing our personas, they will be our guinea pigs throughout this article.
We have:
Junior, our junior who has 2 years of XP in the code. He has just left school and is very very motivated
Chrichri, our lead dev, an old timer with 15 years of XP
Our senior dev team, the no-names. The brute force of our team

Captain Obvious

We are going to start by pointing out what should be obvious, what should be one of the first things that our engineering schools teach us.
This evidence is a truth that must be accepted as quickly as possible in order to successfully experience the exercise of code review.

Come on, enough suspense: “We are not our code”.

It may sound silly, but it's true. If so yes.
Our code is the result of a combination of circumstances, it depends on (and this is a non-exhaustive list!):
Our knowledge at the time T when we wrote it
Context (Was the environment noisy? Did we have a 3-year-old child in our legs because we have all been confined for months?)
The time we had to allocate to this feature (we see you project managers)
From the existing codebase
The quality of the coffee at our disposal (or tea, or beer)

There is also another argument that I find essential in favor of the dissociation of the man and the artist (well, of the coder and his code what): we are much more thing in life than a simple piece of code, and even if having the stamp of our colleagues is a real pleasure for the ego, it is important to keep a distance and put things into perspective.

I am a benevolent reviewer

Take into account the level of XP
The code written by our friend Junior should not be evaluated under the same spectrum as that written by Chrichri. We cannot have the same level of requirement simply because they do not have the same technical background.

Our comments must therefore reflect this distinction.
Be careful, I'm not saying that on the pretext that a junior dev wrote the code so you have to let everything pass. No, I'm saying we can approach things differently.

Chrichri will undoubtedly be able to welcome a comment that is straightforward, with little context, and that goes straight to the point, like the example below. This does not prevent precision and benevolence, just we can afford to develop the message less.

On the other hand, when Junior wrote the same code then we can surely take the time to detail our approach. Explain to him the interest of using a translation file for his character strings, for example.

The imperative you shall not use

You have seen it on the examples above I used the conditional to express my comments.
The reason is that 90% of the time my comments are suggestions, not orders.
Of course, if I see a bug then I would use a different wording to clearly convey the imperative side of the change, but this is the only case where I will allow myself that.

The comment that we put is above all there to help our valiant developer to initiate a reflection, even a discussion, and I am absolutely convinced that it is impossible if we give orders!

And then, I don't know about you, but I will very very quickly be annoyed if my dev friends spend their time telling me what to do and how to do it ;-)

Source?

Let's put ourselves in situations:
Junior, our beloved junior, submitted a PR regarding a feature that generates statistics for a country on the 1st of every month.
He wrote a “getStatsForPastMonth” method which takes the ID of a country and a date as parameters.
The prototype is therefore the following (sorry for the less technical among you):
public function getStatsForPastMonth(int $countryId, DateTime $fromDate)

In the review, of course Chrichri will ask him why he felt the need to integrate the second parameter, since the business specification indicates that we work on a fixed date.

Junior replies that he did this to make it possible to write unit and functional tests that are repeatable regardless of the day the test is run (let's all take a moment to congratulate Junior for integrating tests and thinking about their reproducibility!).

Chrichri therefore finds himself here with a choice. He can respond to the choice:

“You shouldn't change your method to make testing easier. You have to remove the $fromDate parameter and mock the date instead”
“You shouldn't change your method to make testing easier, that's really not good practice. You should use a mock instead. Here is a link to the library I have in mind and an example I have already written on such a project. On the other hand, you will have to modify the way in which you instantiate such a variable because it will not be compatible”

In your opinion, which choice will allow Junior to improve his skills and correct his code with the least amount of back and forth possible?
The question was quickly answered, especially since it took Chrichri a few more minutes at most, and saved Junior hours of migraine!

I have voluntarily taken the example of Junior here because it is easier, but nothing prevents us from having this kind of feedback with all the developers of course. The goal of a code review is to help others progress and not just to ensure the quality of the code base.

Don't be so negative!

It's harder to compliment than to criticize, and that's a shame.
This paragraph will be very short. It boils down to telling you that I love you. And that you shouldn't hesitate to tell your colleagues, especially during a code review.

Let him know that you find his implementation super cool or that you learned something. It boosts the ego and it is as good for the one who wrote the comment as for the one who will read it.

Love yourself, damn it!

I'm a happy developer

Now let's try to ask ourselves how, as developers, we can better experience code reviews.

We are never better served than by ourselves!

I have a reputation for being lacking in rigor. Not necessarily in my code, but often in the sidelines (document, swagger annotations, typos …) and that made me dread code reviews.
Then I was given a great piece of advice: take the time to do my own code review.

I take about ten minutes to read my code, but not in my IDE, no, directly on Github or Gitlab. It allows me to change perspective and spot mistakes much more easily.
And, generally, this proofreading allows me to save a lot of time that I would have lost processing feedback (and feeding my impostor syndrome to the detriment of my ego).

Make life easier for the reviewer

There's nothing like being in a bad mood starting a code review than having no idea what it's about, having to look up the ticket on Jira and then having to figure out how to make that damn piece of code work on your own. !

Always take the time when writing a PR to think about how our peers will be able to work effectively.
So take the time to include a description containing the essentials to understand the business and be able to test the code functionally.
It's not about order
I know, I've said it before. And no, I'm not rambling.

The reviewer has made a lot of feedback, preferably with a benevolent tone (but he may not have read this article!) and you tell yourself that, indeed, a lot of his feedback is well-founded.
But, and we all know it, the life of a developer is not only governed by our desire to do well. There is a reality of projects, deadlines, ticket estimates… in short, we don't always have time to do everything.
Or maybe you think not, his comment telling you that you should rather name your variable $foo when $bar suits you perfectly is not justified.

If you are in this situation then I have a very, very important thing to tell you. This is a secret that has been too well kept until now.
You have the right to say no. Yes yes, you read correctly. It is completely ok to say no to a return on a PR.

Of course, I do not advise you to answer a "no" as an answer but on the contrary to accept this invitation to the discussion by explaining, briefly, why you think that it is a mistake, or why you think your way is better.

On the other hand, if your only argument is the time that the fix will take you, then you have to be careful. Putting it off often means never fixing it. I can only advise you too much to talk to your project manager to find the time you need. After all, project managers are empathetic, benevolent beings who only think about your well-being and your happiness, so everything should be fine. But that is a topic!

Ask for help

Whether you are a junior or not, you have to know how to ask for help, and this applies perfectly to benevolent code review.
If the return seems insurmountable to you or you have no idea how to do what you are asked to do, then do not hesitate to ask for help. It can be a point for the person who gave you the feedback to explain to you what he has in mind or downright a pair-programming session to help you implement it.

You can also request that the review be done directly in pair-programming. Very often, this saves a lot of time and makes the exercise much more humane.

I want to end this love-filled article by inviting you to remember that we all do our best. Whether it's the proofreader or the proofreader, we all make mistakes and we always will.
Let's be indulgent and kind with others, of course, but especially with ourselves