The Best Code Review
This article tells the story of the best code review style I know and discusses the key points you should focus on while reviewing code.
Join the DZone community and get the full member experience.Join For Free
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 profession, 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 have had the honor to work with.
Besides having over 15 years of experience, Alin was one of the founders of his company. However, what made Alin unique was his mindset of continuous learning and his great soft skills.
Our Code Review Story
You just finished your task and pushed the code to the team repo. You aren't very experienced, let's say you have three years of experience, but that's rather the norm in our field today.
You did your best to write clean, simple code, but, despite that, you can't say that you're incredibly proud of the result. The changes you had to make in the old code were complicated at times and you found yourself having to take some shortcuts; but you really don't see any better way.
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.
This is the story of every evolution in large, pre-existing codebases: a 15 year-old codebase, 1.5 million lines of code, with some classes coming in with 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 has some lovely rare spices to choose from). If you're not a tea person, he offers you chocolate, an apple, or anything tasty he knows you prefer.
Alin already has your changes merged in his local workspace, and you can tell that he studied it a bit before he called you over. 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 of extracting 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've ever seen?
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. 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 is truly happy to answer any of your questions.
But now it's 90% done. Far better than the original code you submitted and you approved all the changes. You felt in charge the whole time. Yes, Alin provided the ideas and applied them, but you decided which ones to use, 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!
Right when it's almost ready to commit, when all the pieces are nicely laid out in the code, when it's all clear in your mind...
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 my code being refactored into something far better right in front of me. It was a strange mix of being astonished by the patterns and simplicity of the resulting code and shock at losing it all!
But, back to our story.
You go 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 two 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 took. It doesn't matter, though. The code looks way better, and now you are proud of your work.
When Alin reviews the next version of your code, he praises your efforts and accepts it, even if it's still imperfect in his perspective.
His magic worked.
You did your best!
The Method to Alin's Code Review Madness
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 decisions of the author(s). It turns out that he wasn't looking for 'impeccable design,' but for 'your best design.' In other words, he had different expectations for different team members.
Of course, if there was an 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 more subtle or nuanced coding practices, he always preferred to 'leave it your way' if you didn't agree, and preserve a positive attitude rather than arguing over some semantics. His long experience taught him that not all design "flaws" grow into real issues.
So be kind to the reviewee. Offer them some tea and some nice ideas.
That's why I don't like written reviews. It's very difficult, if not 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 offer a comforting non-verbal gesture. So please, try to conduct your more elaborate/delicate code reviews as live meetings whenever possible.
Learn, Don't Blame
"The purpose of code reviews 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 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 chosen solution, so you can see the big picture more clearly.
Quite often the reviewer ends up learning something themselves. 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, and teaching.
Some issues might arise in multiple code reviews. When this happens, discuss 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. It has obvious bugs, bad names, and other issues that will immediately sound the alarms of any experienced code reviewer. Perhaps the author had a bad day, they couldn't focus, or maybe they were tired or rushing.
Or perhaps that developer has just joined the team, and is thus inexperienced and unsure. When this happens, switch to a more 'guided' teaching method and gently walk them through the significant issues, correcting and deeply explaining each one.
Throughout this article, I have assumed a positive team mindset and that everyone is trying to do their best. 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 can 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.
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 team spirit.
- Submit reviewer-friendly pull requests with explanatory, fine-grained commits.
Pair programming is a good alternative to code reviews. If you paid attention, Alin 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?
He is, or at least 90% of this story is real. I just added some techniques to his character that I've seen used by other remarkable technical leads I've met over the years. But I believe Alin remains a tangible ideal.
Published at DZone with permission of Victor Rentea. See the original article here.
Opinions expressed by DZone contributors are their own.