EXPEDIA GROUP TECHNOLOGY — ENGINEERING

The Importance of Being a Code Reviewer

A framework for giving feedback to others’ code

Photo by davide ragusa on Unsplash

Feedback is like a steak — if it’s too raw, no one likes it, but if it’s overcooked, it’s hard to swallow
(ChatGpt)

Reviewing others’ code consistently can improve your skills and leverage your career. You can help others grow, as well as give value to the company you work with.

In this article, we’ll see the benefits of reviewing code in a pull request (PR), and some advice on the approach to follow when doing it, like interacting with other colleagues.

Definitions

Code Review: the process of giving written feedback to an author’s branch

Pull Request: the GitHub term used to indicate the differences between a new branch and the main branch, where you can leave your comments

Code reviews? Isn’t it in the past?

If you’re an Agile developer, you may question the practice of reviewing code. For example, your observations may be:

• If a consistent pair programming activity is put in place, there’s no need for a formal Code Review process, where a “stranger” comes in and judges your code, often without any context, after all the work you’ve done and the time you spent.
• The correctness of the code should have been assessed already, using a TDD approach.
• The syntax and style can be automatically checked by a linter.

These surely are valid considerations. However:

The Code Review process is essentially a knowledge-sharing mechanism between members of the team and between different teams¹

Benefits

Doing code reviews can bring lots of opportunities if you stop seeing them as an imposed duty, or a boring task.

To show this, I came up with a framework that can help in the process of giving feedback to a colleague’s pull request.

W3H: why, what, when, how

Even if I’m not a huge fan of acronyms, I coined the W3H one to summarize the important questions you may ask yourself when approaching someone else’s code.

Why

Doing code reviews can be enforced by your company’s CI/CD processes, so the “why” part could be answered with “because my organization told me to do so”.

However, it’s beneficial to ask yourself: “Why is it considered so important ?”, or “Why should I do code reviews proactively ?”

First of all, it’s a learning opportunity for both the reviewer and the author.
An author can get hints and suggestions that can improve quickly their skills. The reviewer can learn new tricks and idioms of the language used.

Furthermore, as a developer, you can:

1. Understand faster a new codebase.
2. Do networking, especially if teams are geographically distant.
3. Find potential overlapping and duplication in work from other teams.
4. Enforce best practices in the codebase.
5. Be recognized as an expert and knowledgeable person by other engineers and managers.
6. Strengthen your communication skills (we’ll tackle this in the “How” section).
7. Make friends!

What

What is a “good enough” code?

Before you write any comments on a code written by someone else, you have to be clear on the minimum requirements a good code should have.

Let’s provide some general rules:

1. There shouldn’t be obvious errors, like typos in names, or nonstandard indentation. I usually do this kind of check to “warm up” when looking at a PR on a module I don’t know well.
2. The code should be structured and easy to understand, even if it accomplishes complex tasks.
3. There shouldn’t be major performance issues, e.g. a list read multiple times when it could be with a single cycle. This is true especially for mobile apps where the risk is to consume unnecessary battery power.
4. A PR should only change the files necessary to accomplish the task (feature, bug fix, refactoring). This will not only ease the work of the reviewer, but it will be easy to spot and revert in case of a major production issue. If the code touches multiple aspects, then suggest the author to split it into two or more reviews.
5. Understand the goal of the change. The PR description should describe the change or link to an external document with details (for example a Jira or Trello ticket). Once you understand the goal, check if the change matches the requirements.
6. If the PR addresses a bug, the fix should also introduce a set of tests to address that specific scenario and avoid further breaks in the future.

When

When should you spend time reading code?

I usually devote around 30 minutes, twice a week, for PRs from other teams, so not related to my team’s work. It can be less if there’s a stringent deadline for my team, or it can be more if it’s a quieter day and the comments generated a lot of debate.

When I first started my journey in the iOS world, I used to spend time in code reviews as much as in studying Swift and iOS. This allowed me to get up to speed quickly to the new codebase, language idioms, best practices, and to know relevant people in the project.

I usually start code reviewing as soon as I have to learn a new large codebase

How

Generating hundreds or thousands of comments may get boring and repetitive somehow, and you may tend to be too direct. Written communication is different from verbal, so I would suggest following the advice below:

Be kind
This is self-explanatory. Usually I start off a review to someone who I don’t know yet with a simple “Hi 👋” in the first comment. If you spot something missing, don’t express this with a statement such as, “This is incomplete”, but rather, ask: “Is there a reason why this is missing?”

Attention to detail
Try to balance between important and not-so-important changes (experience will guide you). We don’t want to block an important feature or production bug because of a spacing issue!

Ask the author’s opinion
If the change is not obvious, or you’re requesting refactoring, it’s good to end your thoughts with “What do you think ?” or “What’s your thought about it ?”

Tradeoffs are inevitable sometimes
Some projects are more critical than others. Try not to insist too much on small changes which could risk a delay in the release. You can agree with the author to create a “polishing” branch later on. However, as I often say:

later == never

Don’t make assumptions
You’re inspecting a PR, and you spot a macroscopic error. Plus, the dev who’s written the code has a “junior” title. You might assume that that error is due to inexperience, and the lack of knowledge of some basics. Maybe it was written in a hurry, or the commit came after a hard day’s night.² Or … chances are there is a hidden, logical motivation behind that. If in doubt, tell the author: “Maybe I’m missing something, but …”

Keep personal preferences optional
When you suggest changes in code reviews, make sure that these changes are real improvements to the code and not the reviewer’s personal preferences, which are not necessarily the same as the company or industry best practices. I use this formula when I propose such a change: “This is a matter of personal preference, but if you like it <code change>”

Compliment the author
“LGTM” (looks good to me) might be interpreted as “I reviewed your PR quickly”. Personally, I’m ok when I read that, however, don’t hesitate to write down in the PR how well the code has been written (If you genuinely think it is true!). Here are a couple of reasons why a praise is well deserved: a new fancy functionality has been introduced, or a complex yet well-architected refactoring occurred.

If the pull request contains a simple color change or a parameter addition to a function, then an extremely enthusiastic compliment might be interpreted as sarcasm 😅. Calibrate your words carefully.

Side effects

It doesn’t matter how kind you’ve been, or how much you’ve contributed to improve the code during a review, sometimes people will feel upset. Maybe they will make the requested changes, but won’t thank you for having devoted your time to their code.

Someone will reject your suggestions
Maybe your suggestions are simply wrong, or useless, or maybe the author’s ego is suggesting that their changing the code will undermine their self-esteem. That’s ok. However, if you think a snippet of code is dangerous or can introduce serious performance issues, consider involving other devs in the discussion, by tagging them. Even better, try to discuss in a call or in person.

Don’t expect to be rewarded for the effort you put in
Even with tens of code reviews and approvals that helped people to merge their stuff, when it’s your turn, don’t take for granted that you’ll receive attention to your shiny new pull request.

You can win allies
Last year, I did consistent reviews on a codebase I was new at, with the goal of getting quickly onboarded to it. There have been engaging discussions and proposals with many devs and, eventually, some modules were changed and improved. That was a result of a joint effort of the author and me — the peer reviewer, a perfect stranger! I met great people, who were happy to improve their code and remained always open to suggestions and learnings. At the end of the quarter, when I asked them for formal feedback, I received fantastic responses. Some call this networking, I could even use the word “friendship.”

Conclusion

In this article, we discussed the benefits of giving consistent code reviews.
I promoted my W3H approach, which defines a process for giving feedback to someone else’s code.

I apply this regularly in my daily job at Expedia Group™️, where collaboration is incentivized and formally defined by our “Include Consciously” value.

To sum up:

1. Reflect on the benefits of looking at someone else’s code (networking, onboarding in a new codebase, learning new idioms, and so on).
2. Do it often.
3. Give feedback beyond your team boundaries.
4. Don’t make assumptions.
5. Don’t expect the same amount of dedication to your pull request.
6. Be kind.

Thanks for reading so far! If you have any questions or comments, please leave them below. If you liked this article, consider clapping 👏 and following me. Thanks!

Further reading

• The Pragmatic Programmer: Your Journey to Mastery, 20ᵗʰ Anniversary Edition, David Thomas, Andy Hunt, Addison-Wesley, 2019
• Agile Technical Practices Distilled, Pedro M. Santos, Marco Consolaro, Alessandro Di Gioia, Packt, 2019

P.S.

This article was not generated using artificial intelligence. I used only my limited human brain power, work experience, and struggled to find the time to write it.

Learn more about technology at Expedia Group

[1] This definition and some useful insights about how to produce better pull requests and have more chances to receive a successful review quickly, are shown in this really interesting article by Mario Bittencourt.

[2] If you guessed the citation without searching the web, write it in the comments, and you’ll make me a happy dev 😃.