The Best Code Review

This article tells the story of the best code review style I know of and discusses the key points you should focus on while reviewing code.

By the nature of my activity, I get to work with lots of developers, some of which are their companies' thought leaders. This story describes the code review style of Alin, one of the best developers I had the honor to work with.

Besides having over 15 years of experience, Alin was among those developers that started the company many years ago. However, what made Alin unique was the mindset of a continuous learner and the powerful soft skills, which you'll see in action in the following.

The Story

You just finished your task and pushed the code to the team repo. You aren’t very experienced, let's say three years of work, but that’s rather the norm in our branch today.

You did your best to write clean, simple code, but despite that, you couldn’t say that you’re incredibly proud of the resulting code. The changes you had to do in the old code were sometimes complicated, that you had to take some shortcuts, but you really don’t see any better way, to be honest.

Then, somewhere during those many hours of coding, you had this awkward feeling that you were reimplementing stuff, but you couldn’t find any existing code doing precisely what you wanted.

And then, just before you completed the task, you had a glimpse of a deeper refactoring that could greatly simplify your implementation, but you weren’t able to grasp the whole picture. There were too many parts involved, too many unknowns. But all in all, the new code that you wrote was quite neat. Only the integration points with the existing code were somewhat flaky.

Sounds familiar?

This is the story of every evolution in large existing codebases. They had a 15 years-old codebase, 1.5 million lines of code, with some classes over 10.000 lines, but you can get the same feeling in much smaller codebases, of course 😉.

Exhausted from all the searching and browsing but somewhat happy that you completed your task, you pay a visit to the ping-pong table.

After recovering some of your mental strength, you return to your desk. Half an hour later, Alin waves at you, calling you to his desk in the middle of the open space.

Next to his chair, Alin keeps a very comfortable armchair, more comfortable than his own, in fact. Alin invites you to sit down and kindly offers to prepare you some tea. And he does have some lovely rare spices to choose from :). He would offer you chocolate, an apple, or anything tasty he knows you prefer, otherwise.

Alin already has your changes merged in his local workspace, and you can tell that he studied it a bit before he called you. He starts by telling you in a relaxed tone that he's quite happy with your code. Then he asks you: “But what do you think if we extract that part into a separate function?....” And he waits for your approval. Yeah, sure, you like his idea.

Did I mention that Alin was among the fastest developers I ever saw?

Right there, in front of your eyes, he quickly performs that change.

“How about introducing a new Value Object for that workflow?” - Alin asks, and he waits for your answer.

If you hesitate or you can’t see his point, he immediately sketches the change in front of you to clarify it. It takes him several seconds, a minute max, but it’s all crystal clear now.

But if you still aren’t convinced about this change, if you don’t honestly like it, Alin simply reverts the change and drops the suggestion, even if the design is imperfect according to his high standards.

After an initial series of quick cleanups, Alin discovers that one small function he extracted is very similar to another existing function - in the refactored code, it's now obvious how to reuse it

And then you suddenly see it: that broader refactoring that you foresaw. There it is, lying in front of you. Alin quickly sketches a little diagram about it to brainstorm over it. Then, after a few more IDE refactorings, the job is 90% done.

Oh, and if you ask Alin to explain one of the refactoring spells he used, he would immediately stop, CTRL-Z a bit, pass you the keyboard, and tell you what to press to do it with your own hands, two or three times. You are always confident that Alin would be truly happy to answer any of your questions.

But now it’s 90% done. Far better than the original code you submitted, and all under your commands. You felt in charge the whole time. Yes, Alin provided the ideas and applied them, but you decided which ones, brainstorming all the way. You are still the author.

And it’s almost there. You know that if Alin would spend ten more minutes on it, maybe a bit more on the tests, he would finish it all. Right there, on the spot!

But no!

Right when it’s almost ready to commit, when all the pieces are nicely laid out in code, when it’s all clear in your mind...

Right then...

Alin reverts all his improvements and tells you: “Now you do it!

YOU ARE ABOUT TO SCREAM: how could you?!?! It was almost done! Why did you do that!?!?!

"So that you learn to do it yourself!" - Alin replies

You storm away, rushing to improve your work, full of ideas and energy.

I still get goosebumps when I remember how it felt. To see your code being refactored in front of you to a far better shape. Being astonished by the patterns and simplicity of the resulting code, and then losing it all!

You come back to your desk, you take that code, and you repeat all that refactoring Alin did with your own hands, starting from scratch. It takes you 2 hours, not 20 minutes, plus more time for the tests. You aren’t as fast as Alin, but you did use one new IDE trick he showed you. The diagram that you brought back also proved useful. Of course, along the way, you forgot several steps Alin did. It doesn’t matter, though. The code looks way better, and you are now proud of your work.

When Alin reviews your next version of the code, he praises your efforts and accepts it, even if it’s still imperfect in his perspective.

His magic worked.

You did your best!

Key Points

Be Kind

I asked Alin: why do you allow flawed code to pass your review? It’s not all about the end code, he said; I’m more interested in encouraging people to do their best and make conscious decisions.

Alin always respected the decision of the author. It turns out that he wasn’t looking for ‘impeccable design’ but for ‘your best design’. In other words, he had different expectations from different team members.

Of course, if there was one obvious error or bug, he would fix it on the spot, and then he'd explain the problem to you. If the design changes were more elaborate, he would rely on the review style you saw above. But if his suggestions were only subtle aspects, he always preferred to ‘leave it your way’ if you didn't agree, and preserve a positive attitude than arguing over some subjective topics. His long experience taught him that not all design "flaws" grow into real issues.

So be kind to the reviewee. Offer him/her some tea and some nice ideas.

But it’s very difficult (impossible?) to show kindness in a written review. Written words tend to weigh more, they can hurt more, or be misinterpreted. You can’t make them soft with a warm tone, or a non-verbal gesture, or mimic. So please, try to conduct your more elaborate/delicate code reviews as live meetings whenever possible.

Learn, not Blame

“The purpose of code review is learning not blaming” became a viral post on LinkedIN, with more than 150K views.

Blaming the code or the skills of the author will frustrate people, discourage creativity, and poison the team spirit. So be very careful with the words, tone, and attitude you have in your reviews. Practice it in a mirror, or record and carefully inspect your attitude afterward.

But learning in a code review means much more than just the reviewee learning from the reviewer. When reviewing code, the reviewer should aim not only to find apparent flaws and bugs (tools can help a lot with that), but the effort should be put into understanding the solution chosen to the problem, to see the big picture.

Quite often the reviewer ends up learning himself. If both the reviewer and reviewee have comparable experience levels, both may learn technical things from one another. But when a senior reviews a junior, the senior needs to learn and practice soft-skills like kindness, empathy, teaching, and putting oneself in the shoes of the other.

Some issues might arise in multiple code reviews. When this happens, discussed them in a Team Design Meeting and perhaps insert them into the Coding Guidelines Document, about which I will blog soon.

One last thing. It hurts to say it, but sometimes, the reviewed code is just bad. Has obvious bugs, bad names, and other issues that will immediately ring the bells of any experienced code reviewer. Perhaps the author had a bad day, [s]he couldn’t focus. Or [s]he was tired or rushing. If you are sure that [s]he can do better than that, ask the developer on a funny note: “Hey, X, should we add this code sample to your CV? Is this your best?”. Give him/her a chance.

Or perhaps that developer just joined the team, inexperienced and unsure. When this happens, switch to a more ‘guided’ teaching and gently walk with through the significant issues, correcting and deeply explaining them.

Throughout the entire article, I assumed a positive team mindset, that everyone is trying to do their best, at least when asked. Building such an attitude has a critical influence on how much effort developers put into their work. Improving that mindset might be a topic for a later blog post.

Help the Reviewer

If the reviewers are typically the more experienced developers of your team, then you have to mind their time, even from a purely economic point of view.

Here are some tricks you could try when authoring larger-scale changes:

  • Break them into separate steps, and ask for an in-flight review.
  • Create many little commits; don’t squash them before review.
  • Commit massive refactoring separately if performed with safe automatic refactoring tools, to allow the reviewer to overlook those giant but low-risk steps.
  • Sketch a diagram for the more complex design and link it to the pull request.
  • Offer to provide a walkthrough to the reviewer to explain your ideas.

Conclusions

During a Code Review:

  • Be kind and humble, both when reviewing and being reviewed
  • Run complex reviews via live meetings: in person or audio+video calls
  • Discuss the code constructively
  • The author should have the final word
  • Both sides should Focus on Learning
  • Concentrate on facts, not on opinions or style
  • Accept ‘imperfect’ solutions to preserve the team spirit
  • Submit reviewer-friendly pull requests with explanatory fine-grained commits
  • Be kind

Pair Programming is a better alternative to Code Review. If you paid attention, Aling really converted a cold code review into a constructive ad-hoc Pair Programming session. I will be blogging about Pair Programming soon, so stay tuned.

Disclaimer: Is Alin real?
It is. 90% of it. I just added to his character some techniques used by other remarkable technical leads I’ve met over the years. But I believe Alin remains a tangible ideal.

Popular Posts

3 Comments

  1. Thank you for this blog entry, it’s really cool and educational. It’s interesting how you and Michael Lynch do not agree on one point (https://mtlynch.io/code-review-love/): “The author should have the final word” (from your blog entry) vs. “Award all ties to your reviewer” (from Michael Lynch’s).
    My opinion on that is that it needs to be determined at the organization-level (team, etc.) beforehand.

    1. That’s actually exactly what made it unique: YOU were in control. However, in practice most developers would end up following closely the suggestions of Alin; but the feeling you got was that of a deep respect and kindness.

      One additional important point is that Alin won’t ever insist on subjective aspects. So for a tie, the author always won.

      There might be one more factor into play: time. Alin somehow always managed to find time to do a detailed code review with deep explanations. If he would rush, I guess he would have to “override” sometimes the decision making.

Leave a Reply to Raymond Ros Cancel reply

Your email address will not be published. Required fields are marked *