Code reviews are a big part of writing software, especially when working within a team. It is important to have an agreed-upon etiquette for reviewing code within a team. A code review is a critique and a critique can often feel more personal than the code writing itself. A sloppy, under-researched, or insensitive code critique can cause difficulties between team members, reduce overall team productivity, and diminish code quality over time. This post will briefly define code reviews, describe some common mistakes, and provide some quick tips for improving a code review process.
What are code reviews?
Code reviews are the process of sharing code so that other engineers can review it. Code reviews can happen verbally during pair programming sessions, or through reviewing code on websites like CodePen and GitHub. Mainly, code reviews happen in tools like GitHub when engineers submit pull requests.
Critiques are hugely beneficial. Convening engineers to discussions about code ensure that they’re on the same page, regardless of whether it’s in person or by sharing comments. Also, a review can help catch small mistakes in code or comments—like spelling and it can help newer or more junior coders learn the codebase. When done well, regular code reviews have nothing but benefits for all involved.
A common goal for code reviews is to make code changes as minimal and clear as possible so that the reviewer can easily understand what has changed and what is happening in the code. If code reviews are smaller, they’re more frequent — potentially several a day — and more manageable.
Reviewing code should be a part of every developer’s workflow. Senior reviewers are given the opportunity to teach/mentor, and even learn something new from time to time. Junior reviewers can grow and often help ensure code readability through the questions they ask. In fact, junior engineers are usually the best team members to ensure code readability.
For an engineer who works alone, asking for feedback from outsiders — at meet-ups, GitHub, Open Source Slack Channels, Reddit, Twitter, etc — can allow the solo coder the opportunity to participate in a code review process.
If we could all agree on an established process and language for reviewing code, then maintaining a positive environment for creative and productive engineering is easier. A code review etiquette benefits everyone — whether working alone or within a team.
Harsh code reviews can hurt feelings
Seeing bugs and issues continue to roll in and being mentally unable to address them has led to feelings of failure and depression. When looking at the moment project, I could only see the negatives. The bugs and misnomers and mistakes I had made. It led to a cycle of being too depressed to contribute, which led to being depressed because I wasn’t contributing.
– Tim Wood, creator of Momentjs
There are many online comments, posts, and tweets by prolific engineers expressing that their feelings have been hurt by code reviews. This doesn’t directly mean that reviewers are trying to be mean. Feeling defensive is a normal, quite human reaction to a critique or feedback. A reviewer should be aware of how the pitch, tone, or sentiment of their comments could be interpreted but the reviewee — see Occam’s Razor.
It's like these commenters don't consider maintainers to be people, we make mistakes. https://t.co/tBa8kzymU4
— Henry Zhu @SF (@left_pad) September 18, 2017
Although reviewing code is very beneficial, a poor or sloppy review can have the opposite outcome. Avoid criticism without providing context. In other words, take the time to explain why something is wrong, where it went wrong, and how to avoid the mistake moving forward. Showing this level of respect for the reviewee strengthens the team, improves engineering awareness, and helps to provide agreed-upon technical definitions.
Quick tips for improving code review etiquette
Code is logical in nature. It is easy to pinpoint code that is incorrect or could be improved, just like it is easy to notice spelling misteaks. The human condition, when looking at and discussing logical things (like code), is to disregard the feelings of other people. This causes feelings to get hurt and a loss of focus on learning and collaboration.
Improving code review etiquette, because of the human condition, is difficult! Here is a quick list of things that I’ve done, said, seen, or received, that are easy wins in the art of Code Review Etiquette.
Remove the person
Without realizing it, engineers can neglect the difference between insightful critique and criticism because of personal relationships in communication.
The lines below dissect a code review comment of a theoretical function
where it is suggested that there is an opportunity to return out of the function early.
You and I: Using you or I is probably not offensive intentionally, so don’t worry. However, over time, involving the person can start to feel less positive—especially if vocal tones are ever added.
You should return out of this function early
We: Using we
is inclusive and a safe way to say something more directly without making someone feel defensive. However, if the person speaking says we
, and has not worked on the code at all, it may seem falsely inclusive and insensitive.
We should return out of this function early
No personal reference: Without personal reference, conversation or review will closely communicate the problem, idea, or issue.
Return out of this function early
Notice how the amount of text needed to communicate the same thing without using personal references takes fewer words and has greater clarity. This helps with human interaction, separates code discussion from personal discussion, and fewer words are needed to communicate the same idea.
Keep passionate conversations quiet
Passion is an important motivator for improving. Passion that is critical in nature can be very considerate and motivating. Feedback that is critical in nature is most useful if the person receiving the critique is engaged. This sort of communication comes up a lot during architectural conversations or when discussing new products.
Feedback that is critical in nature is most useful if the person receiving the critique is engaged. Note: the person receiving the information must be engaged with the critique.
Imagine this comment when stated with exaggerated physical movement, more excited vocal tone, and higher volume.
There are 8 web fonts used in this mock which may affect page load speed or even certain tracking metrics that could be caused by new race conditions!
Then, imagine a similar comment, even terser but stated with a calm demeanor, slower delivery, and a normal vocal volume — followed by a question.
There are 8 web fonts used in this mock. This will affect page load speed and possible tracking metrics because of potential race conditions. How can this be improved?
Notice how the comments above are almost the same. The second comment is even more direct. It states a problem as a fact and then requests feedback.
An important thing to remember when being passionate is taking on a quieter tone. This is a physical decision — not a social one. Passionate language can be the same, and perceived very differently, based on the orientation of the communicator’s tone. If physical tone (body language), vocal tone, vocal pitch, and vocal volume remain gentle, it is observed that it is much more likely for an audience to remain engaged — even if the critique is critical in nature.
If the tone is aggressive in nature (exaggerated physical movement, more excited vocal tone, higher volume), the actual words used can be gentle in nature, but the audience can feel very differently. This communication can lead to embarrassment, a disengaged audience, and even loss of respect.
Aggressive communication is common with passionate communication because the human condition wants to protect ideas that we’re passionate about. So, don’t worry about it too much if you observe that your audience is disengaged when discussing something that you’re passionate about. The key is to remember that if you can create perceived gentle communication, it will be easier for your audience to remain engaged — even if they are not initially in agreement.
Don’t review the author, review the code
Following the conversation above, the act of pointing, within written conversation or actual body language, in almost any situation is not optimal for good communication. It changes the focal point of the conversation from the context of the conversation to a person or a thing.
The response below provides a comment and then a link. In the context of the code review, the second part of the comment and link takes the reader out of the context of the code review, which is confusing.
// Return out of this function earlier
// You need to learn about functional programming
The comment below provides a comment, and then a pseudo-code suggestion.
/*
return early like this:
*/
const calculateStuff = (stuff) => {
if (noStuff) return
// calculate stuff
return calculatedStuff
}
In the two examples above, the first example causes the reader to go far beyond the issue. The conversation is more abstract—even existential. The second example refers directly to the issue and then provides a pseudo code snippet that relates directly to the comment.
It is best to only comment on contextually specific items when reviewing code. Broad comments lead to a loss of context. If broader discussions must happen, they should happen outside of code reviews. This keeps the code review clear and scoped to the code that is being reviewed.
Right and wrong can change
Developers almost always want to re-write things. It is natural to break problems down into tasks in real-time to address today’s situation. However, focusing on the who’s and why’s of a product’s history is important to conceptualize because great context is gained. ‘History repeats itself’ is an important phrase to remember when critiquing products or when a product you’ve written is critiqued. There is always a great amount of knowledge to be gained from historical context.
JavaScript was made in a week, considered a hacky scripting language, and then became the most widely used programming language in the world. Scalable Vector Graphics (SVGs) were supported in 1999, pretty much forgotten about, and now, they continue to gain popularity for the new opportunities they provide. Even the World Wide Web (the Internet) was meant for document sharing with little anticipation of the current result today. All of these technologies are important to remember when considering software and engineering—as logical and predictable results are supposed to be, success is often derived from unexpected results! Be open!
Some resources and tools that can help with code review etiquette
- Alexjs: a tool for catching insensitive, inconsiderate writing by Titus Wormer
- Grammarly: an extension to help improve communication in browsers
- Write Good: a cli and text editor plugin by Brian Ford to help improve communication in text editors and shells
- Awesome Writing Tools: an Awesome List of tools for improving writing.
Conclusion
The list above includes general, high-level things that can help with positive engagement when talking about, reviewing, or reading about code—code review etiquette.
I am a hypocrite. I have made all the mistakes that I advise not to do in this article. I am human. My goal here is to help others avoid the mistakes that I have made, and to perhaps encourage some behavior standards for reviewing code so that engineers can more openly discuss code with less worry about being hurt or hurting others.
Great article – thanks. So much here that I agree with. On a similar note, I wrote https://capgemini.github.io/learning/better-learning-code-reviews/ – please take a look
Awesome stuff, Malcolm.
I really enjoyed your article! I read it right away and felt happy to more people were thinking about similar things. Particularly, for me, the section on tooling led me to click through on things that might improve my process.
Thank you for sharing!
A few other suggestions that I use:
1) I conduct the code review to cover the good, the bad, and the ugly.
2) I ask myself the question “if I had to maintain this code, what would I want to know?”
3) I ask others to speak up if they see anything in my algorithms or structure that I’m doing wrong.
4) I often present alternate implementations with pros and cons (for example, using metadata and reflection vs. imperative code, using LINQ vs. “old style” coding.)
5) And most importantly, I don’t lead code reviews of other people’s code, I ask them to lead a code review of their own code.
#5 Great idea Marc.
This is phenomenal. Really nice explanations. A few additions based on what I have experienced:
Code reviews can be based on management principles that the team is aware of. Otherwise, engineers can be out of sync pretty easily.
Focusing on the substance rather than the style has helped me extract the message the person was sending. I usually first assume the person commenting has the best intention of helping.
Many times code reviews still lead to a lot of contention. Assigning a person that is Believable to resolve disagreements can help. Many times emotional processes of “I just want to get my PR in” or justifying why one did something not based on your team’s principles will lead to contention immediately. If somebody who has shown to solve similar problems consistently and can explain their process well is assigned to resolve questions in a PR, it feels like disagreements lead to members coming together instead of moving apart.
Very nice article. I have a question though.
When working with a team and doing constantly pushes/PRs isn’t it cumbersome to review each pull request? How would you solve that?
As Malcom mentions, great article! Something I have shared with my team.
I am commenting just to let you know I appreciated this article, and value the sharing of less “glamorous” content than the latest tech bling.
I find it funny that working with my own family is a lot harder to maintain my cool and be objective. But since I have been on the receiving end of so much unnecessary personalized criticism, I try to be very careful to make my own critiques of others based on pure logic.
This helps me accept solutions from other people that I may normally not want to deal with, as I have learned to appreciate other people just getting things done, and if they want to use basecamp *sigh*, we will use basecamp. It’s better than poor communication and constant errors. In fact, I am actually happy to be using basecamp for a particular client, because instantly that client started using clear communication. Specifically, in emails it was conversational tone, but in project management software, conversational style doesn’t make sense.
So now that I wrote that out, I wonder how much environment, and perhaps even a critique form (with actually documented requirements) would affect feedback from users.
I haven’t done this, but I am close to building my own forms just for the purpose of critiquing. Including things like “NOTE: Be sure to not include “I/we/she” in your critiques.” Code could even be used to enforce certain fields do not allow those words in them. :) Draconian? Perhaps, but it certainly would get the point across, don’t be a jerk.
And I have found that those that will happily work together with others follow social rules easily, and those that won’t will be angry at being forced to behave no matter what. Maybe a win-win-win.
I’d like to suggest an addition here which has less to do with tone and more to do with content: Avoid requesting lintable changes.
Too many times, I’ve seen people review code with suggestions like “dependencies should alphabetized”, “this could be a ternary”, “use tabs/spaces not spaces/tabs”, etc. Using a linter to enforce this in a project allows the reviewer to focus more on the intent of the code and not the structure.
Such a great point! Thanks for the thoughtful addition.
Awful. You wrote an awful article. Go try again.
Just kidding… I’ve had code reviews like the above before though sadly. Thanks for these awesome tips!
This article is going in the right direction, but IMO you can never eliminate the personal from group code reviews.
Even if you eliminate the accusatory language, it is human nature to ignore the good parts of the code and focus on things that have been done badly, either because the person didn’t know how to do them better, or because they didn’t have time to (usually the latter, because most engineers will try to delve deeper if they’ve got time).
Too often, even the best intended code reviews degenerate into the “rock star” of the team telling other team members how to improve their code.
This cements a hierarchy in the team, and hinders teamwork.
Rock may start feeling frustrated because he’s always giving people the benefit of his knowledge, and they don’t seem to appreciate it.
It also makes people nervous about checking in when they know that Rock will be telling them how they could have done it better with the latest and greatest feature from C# newest version. So then they start double checking stuff, and trying to find out if there’s a better way to do it, which makes them slower, and consolidates Rock’s position as the star of the team, because he (and I’ve never met a female Rock) is the fastest as well as the most competent on the team.
This is always justified with the excuse that “it makes the code base better (unspoken: and people just have to learn not to be so sensitive.)”
But in the end, code is still written by people. Unfortunately, their feelings and needs have to be taken into account. If you don’t agree with that statement, repeat after me: Not everyone is like me
But I’d hazard a guess that there are more “sensitive” people than Rocks in most software teams.
This is why I’m still against code reviews, and I actively avoid job adverts that say in the advert that they do team code reviews.
I’ve never had code reviews in my company, but for different reasons than you present, my experience is that critiques of any kind seem to fall into the same problems.
Whether it’s design, photography, writing, editing, programming, etc… Either from a team member, programmer, employee, client, etc… the effect is the same. Which is why I appreciate what this topic has brought up.
I no more want to be accused of poor quality work, than anyone else, nor would I want a client talking down to an employee, or them to eachother.
You can’t escape feedback, code reviews just seem to be a forced critique. And maybe whether or not code reviews should be done is a whole new topic?
Another nice way to phrase observations in a review is as questions – such as:
“Could we …?”
“What if we …?”
“Would it work if we…”
The nice consequence of this is that rather than dictating a change, you are engaging in a dialogue. Also, rather than asserting superior knowledge (despite inferior contextual awareness!), you are giving the original author the opportunity to explain their rationale, and opening up a collaborative conversation about alternatives.