How to properly give and receive code review (without drama)

Code review is one of the most beneficial and formative tools for developers. Except if it’s done badly. It can quickly become a big drama. There are things to know humanly and technically to succeed in a code review. Whether if you give it or if you receive it.

Litigation

A long time ago, in a galaxy far, far away, I was working in a team of about ten developers. When new code had to be integrated into the system, it first went through a classic code review process.

The jurisdiction for us developers.

And on that day, that’s exactly what happens. One of my colleagues sends his review code to another. The discussions remain between these two people. But this particular review was not like the others.

In the confidentiality of this pull request, a big drama had just begun.

Over the next few days I felt that the atmosphere was deteriorating. Gradually, the faces closed. The communication became slower and slower.

One morning, the colleague who had received the code review turned to me.

“- I have assigned you Mr. X’s code review. I can’t take it anymore, it’s getting on my nerves. There’s nothing going right and he doesn’t do what he’s asked. It’s been several days, I don’t want to have to deal with all this fucking mess anymore.”

I didn’t really have any reaction right away. I still didn’t understand what was going on. When I opened this famous PR on GitLab, I quickly understood.

On both sides, that was exactly what you dont you want to do and say in a code review.

An enormous amount of code. No context. No tests. Violent comments all over.
Endless debates. It had been going on for a week. It was getting bigger by the minute. Fire everywhere and the damage was already critical.

This code review alone will be the starting point of the drama that will transform the team.

Criminal court

If I use the lexical field of justice, it is for a good reason. Everyone sees the code review as a court of law. A case, a defendant and jurors.

Once and for all, we have to stop with this mindset. The code review is not for judging. The code review is used to improve the quality of the application and to share knowledge within the team. More concretely, we want three things.

Identify potential bugs

Error is human. It takes several brains to maximize the chances of avoiding problems. Even with several brains, bugs are always there. And this is proof that code review is absolutely necessary.

Spreading knowledge

I have nothing against a certain ownership on some part of the system by certain people. But what we want is everyone to be able to intervene everywhere. And the code review is just perfect, tailor-made to allow that.

Quality Improvement

Quality is a team effort. A group discipline, a strong tradition that should be savagely defended by everyone. This effort goes a lot through code review.

The code review is meant to be a two-way exchange of knowledge, a discussion, not a criminal court.

Bring in the accused

When Mr X saw that the review code was assigned to me, he saw red.

There is a huge difference between a private review code between two people, and a review code that becomes public. Especially when things go wrong.

And the situation suddenly worsened when Mr. X involved management. Pointing the finger at us. I say we, because even though I hadn’t even participated yet, I was already part of the opposing camp.

Pointing at us, he drew a line in the sand. From that moment on, no one crosses that line anymore. It was too late.

It was not anymore about code.

Advocacy

To avoid this hellish situation, here are some indispensable rules for whoever sends a code review.

Sent functional and tested code

It seems obvious. It isn’t. The main problem is the time pressure on developers. If you skip a few basic principles, it’s sure to go faster.

An efficient way to do this and to always put yourself in the position of the person who is going to review your code. Do the code review of your own code as if it was someone else’s code.

Doing that -just that- propels your code review into the top 1% world.

Send as little code as possible

The less code you send, the better. The person who will receive it will be grateful and therefore effective in their review. It also allows you to better control the changes made to your application and limit possible bugs.

An efficient way to do this is to reduce the scope of tasks.

If you see that a task is going to require a lot of code, insist that it be broken down into several tasks.

Send the context first

There is nothing worse than sending code to someone without any context. You’re imposing double work to the receiver. If you want the person’s efforts, you have to make an effort to clarify beforehand.

An efficient way to do this is to put a good description of the problem you are solving in the description of your code review. Why not a direct link to the ticket, if the ticket is well informed.

Anything the person could use to facilitate comprehension is precious.

Taking a step back

A code review aims to challenge your solution. We will challenge your code, not you personally. People who don’t have this perspective are unable to do a code review normally.

An effective way to get there is to remember that you are not your code. That nothing is perfect and that every criticism is an opportunity for improvement. Not a personal attack.

The unbelievable developers ego is strongly tested here.

Jurors

There are also some indispensable rules to respect for the one who receives the code review.

Demand the minimum

To do your review job effectively you need to put up a entry barrier. This barrier should be the same for everyone. You want to optimize the use of your time and the time of the person who sends you the review.

If you see no tests, no context, just politely send it back. Before doing the review, check out the branch and run the tests. Once you are sure that the code you are reviewing is serious, you can spend some time on it.

Ask questions

Many developers forget that the comments they leave are read by a human being.

The way you make your comments is as important as the content of comments themselves. If you’re an asshole, but you’re right, you’re an asshole who’s right. And guess what? Nobody wants to work with an asshole.

I have a very effective way to be tactful in a code review. Just ask questions instead of giving orders. Concretely, instead of commenting “your way of doing this here is garbage, kys” you can say “what do you think about this other way of doing this instead?“

Not only do you not assault your colleague, but you also bring a solution instead of just being critical. Believe me, it’s the secret formula that changes absolutely everything.

You go from being an arrogant jerk to someone who is respected for his tact and technical skills.

Besides, if you are wrong in your analysis and your remark was wrong: it was just a question. You can say that you were just curious and that you understand better now.

Also, whoever receives the code review must master the technical side.
And for this part, you have to start with case law.

Case law

Each team should have a suite of rules that govern how the code should be structured and stylized in the application. This is the source of absolute truth that everyone must respect. The role of the code review is also to ensure that this is applied.

To give you a concrete example, here is the case law for any developer who does Javascript at Airbnb: https://github.com/airbnb/javascript.

The same kind of rules for establishing style rules in your team must exist.

If this is not the case for you, you should propose it.

It will put an end to all unnecessary debates and discussions about the details and style of the code. Now that the case law is applied, we can get to the heart of the problem.

Technically

Now, technically how do you do it? How do you find what to say in a code review? How do you spot questionable code and how do you know what to propose as a solution?

There are two ways to do that.

The first is to expose yourself as much as possible to these code reviews. For years. Learn from developers more experienced than you. Invest yourself in code review. Giving, receiving or just watching them. Doing everything possible to participate strongly in this subject. By dint of it, you will end up having a deep knowledge and sharp reflexes.

But this way takes time. How to go faster?

The second way is a shortcut to the first. This is my recommendation of the day: Refactoring: Improving the design of existing code.

This book is a real gold mine that taught me everything I needed to know about code improvement. Because yes, that’s what a code review is all about. A discussion to improve the code that can be improved.

The first chapters explain a little bit the principle and the main rules of refactoring. It is interesting.

But the most interesting starts in chapter 3, which is dedicated to the formal identification of questionnable code. The Holy Grail, sacred nectar, the source of efficiency of your code reviews.

I devoured this part ! Read it, reread it and reread it several times to make sure I understood everything. I had finally found what to say in the code review. And it’s good to know what to say, but you also have to propose solutions.

Luckily, the last chapter is a catalog of solutions that you can bring to every piece of questionable code. Hundreds of pages organized in an index so you can easily find what you’re looking for. I always have this book near me.

Icing on the cake: all the code for the code examples in this book is Javascript. There are no prerequisites and it is for juniors as well as seniors.

This is a must read if you want to be really efficient in the context of code review.

Damage and interest

Very quickly Mr X’s code review had made the rounds of the open space.

Managers started asking questions. Everybody was pointing at each other. The task was deadlocked.

And I say everyone, because I realized that my colleague who had dropped out of the review, had himself been assigned to it by another colleague, who had dropped out before him.

Of course, since the PR was now public, everyone was giving their opinion on the situation.

The climax was the day of the face-to-face confrontation in the middle of the open space.

The story ends rather badly since Mr. X asked for a transfer to another team shortly afterwards. It was quickly accepted and effective. It was no longer a question of repairing the damage, but of limiting it.

To prevent all this drama, it would have been enough to do just one thing at the right time.

When a drama begins in a pull request, you should immediately close it, and talk to the people in real life.

The dehumanized context of the code review exacerbates reactions, positions and interpretations. The recipe for chaos. The great team destroyer.

Whether you like or not, a code review is strongly about humans and ego. Those who think only about the technical part are bound to produce drama. And it gets exponentially worst when both sides have this mindset.

This is exactly what happened at that time.

This story is not very original. It has already happened in many teams. And without a little vigilance, it will happen in yours.

Epilogue

A code review is not a court of law. It is a knowledge exchange allowing a better quality and stability of your application. And of course, it remains a technical review, but removing and/or trampling on the human factor will produce a chaotic result.