arrow_back

Development

18 Jan 2023

A proposal for clearer code review

Talon.One Writer

Content Team

proposal for a clearer code review
access_time_filled

6 minutes to read

Source control is a collaboration tool

In today’s software development processes, Git is the de-facto standard for source control. Everything revolves around the project’s repository, and whether your team uses GitHub, Gitlab or any of the other providers, software developers will (hopefully) always rely on the following workflow:

  • Create a branch off master

  • Work (fix the bug, build the feature, refactor the legacy code, etc.)

  • Merge the branch back into master

But between point 2 and point 3, any well functioning team will implement this point:

2.5. Code review

Code review is the process in which fellow team members will read your code, assess its quality, and determine whether it is ready to be added to the codebase. It is absolutely vital, as writing software is a team effort, and code review is an important point of collaboration.

This can sometimes be a difficult process, and my article addresses one of the issues I have most commonly faced: how to make sure that the key intent and importance of a piece of feedback is well understood, whether you’re giving or receiving it.

Prioritizing comments is hard

You’ve just sent a pull request. It was a big feature, and there’s a lot of code to read. You’re proud of your hard work and are eagerly expecting the feedback from your teammates. So when it comes, it’s overwhelming.

image

So many comments

You get to work and start to read through all the comments. Some of them are trivial (typos, code style, personal preference), some of them are more serious (security concerns, performance, legibility), and some of them are directly questioning key architectural concepts. And then others that only raise questions that are not strictly related to the scope of the pull request.

You try to answer all of them, but it is hard to keep the focus on what is truly important when you’re following so many issues at once. And it is even harder to keep your teammates informed about the progress you are making. They might ping you to ask about it. Eventually, everything is resolved, and the pull request is merged, but you can’t help but wonder about the time lost by constantly switching context, communicating continuously throughout the process, and how you could do a better job of prioritizing issues next time.

Levels of concern

Not all feedback is equal. Not all issues raised need the same amount of attention. And this can be hard to assess, both when giving and receiving feedback. The first step is therefore to define categories. From personal experience and conversations with fellow developers, all comments on a pull request will probably fall into these broad categories:

Objective issues

These do not warrant conversation: they point out errors or a breach of internal standards. They should unequivocally be fixed, usually in the way suggested. They can be:

  • Typos or oversights (mistyped variable names, forgotten commented code)

  • Code style (not using camelCase for a variable, formatting errors)

  • Mistakes (forgot to return the value of a function, forgot to catch errors)

Expressing concern or asking questions

These usually open a conversation. The reviewer doesn’t understand why a specific problem has been solved in this way and might offer an alternative solution. They can be:

  • Questioning the usefulness of an abstraction, or requesting the creation of one

  • Proposing the use of a library instead of building a custom solution

  • Proposing a simpler, more performant solution

  • Raising concerns about the legibility of the code

Questioning architectural choices

These are the comments you never want to see in a code review. They express concerns that should have been raised earlier in the development process, because they put the whole design into question. They might be:

  • Asking for a functional approach instead of an object-oriented one

  • Asking for a different set of core abstractions (components, classes)

Raising issues unrelated to the current pull request

These are very precious comments that should be taken to heart by the whole team. They might point out some issues or bugs previously unnoticed, but that came up during the review process. They are not strictly related to the current code, though, and should be addressed in a separate pull request. They can be:

  • Shining light on some legacy code that should be refactored

  • Bringing to attention an unrelated bug discovered while reviewing

  • Talking about processes and workflows to be improved

  • Proposing a subsequent feature that this pull request enables

Nitpicks

These should be kept to an absolute minimum on a pull request. They express personal preferences and general bikeshedding. They can be hard to resist as a reviewer… But they should not request changes. Implementation of these suggestions should be entirely up to the pull request’s author. They might include :

  • Use of one library over another

  • Naming of variables when legibility issues are raised

  • Use of one language feature over another

Complimenting the author

These are the comments you definitely want to see in a code review! They point out elegant code and clever solutions, approving the choices made and comforting the author in their skill. They are a very important part of any code review, and vital to healthy teamwork and morale.

As we can see, there is a very high diversity in both the scope and the nature of the comments that we can encounter in a code review. My objective is to make each type of issue immediately identifiable and to ensure efficient communication around it.

Proposal: emoji-based communication

Emojis have become a very significant part of digital communication over the years. In 2015, the Oxford Dictionary even announced 😂 as their Word of the year! They are ubiquitous in the collaboration tools commonly used by software development teams (GitHub and Slack at the forefront). And they are fun ✨So let’s use them!

Categorizing comments ✖️🤔 ❌ 📎 🤷‍♀ 💯

Using a defined list of emojis, we can help every team member to quickly identify the kind of issue a comment raises, as well as immediately clarify the intent of it. Using the previously defined categories, here’s what your list could look like:

  • ✖️ Objective issue

  • 🤔 Expressing concern or asking a question

  • ❌ Questioning architectural choice

  • 📎 Raising an issue unrelated to the current pull request

  • 🤷‍♀ Nitpicking

  • 💯 Complimenting the author

A reviewer would always begin their comment with the appropriate emoji. Like this:

✖️ adminPassword instead of aminPassword.

🤔 Maybe we could use moment instead of implementing our own time parsing.

📎 It looks like this button is bugged in the loading state, I will create a ticket for it and look into the issue later.

🤷‍♀ I really prefer reduce to using filter and map together like this.

💯 I LOVE THIS CODE SO MUCH THANK YOU OMG!

This immediately helps with scanning the comment list and deciding which issue to tackle first, without having to actually read the body of the comments (which can be quite long).

Tagging an issue as “In progress” 👀

So, once we’ve decided on which issue to tackle, it can be useful to inform our reviewer that we have seen their comment, and are working on it. Here, the emoji reaction feature of GitHub helps us. We can simply add 👀 to the comment.

image

Making the reviewer feel seen

Resolving issues 👍

Again, leveraging the emoji reaction feature, a well-placed 👍 lets everyone know that you have addressed the concern and pushed a fix. It should be accompanied by a comment linking to the relevant commit, and might look like this:

image

A happy resolution

It is then the responsibility of the original commenter to either resolve the issue, or to follow through with further comments.

A hopeful outcome

image

✅Merged

In the end, this approach tries to solve issues I have encountered many times:

  • Clearly defining the nature of an issue

  • Smoothly communicating issues in the code review process

  • Making the process easier and more fun!

My proposal of this approach to my colleagues was met with enthusiasm, and I am looking forward to seeing, what I expect to be really great results from it. 😃👍

Monthly loyalty newsletter

Join thousands of marketers and developers getting the latest loyalty & promotion insights from Talon.One. Every month, you’ll receive:

check_circle

Loyalty and promotion tips

check_circle

Industry insights from leading brands

check_circle

Case studies and best practises

Newsletter author

Isabelle Watson

Loyalty & promotion expert at Talon.One

Talon.One Logo

The World's Most Powerful Promotion Engine

BERLIN

Wiener Strasse 10
10999 Berlin
Germany

BIRMINGHAM

41 Church Street
B3 2RT Birmingham
United Kingdom

BOSTON

One Boston Place, Suite 2600
02108 Boston, MA
United States

SINGAPORE

1 Scotts Road, #21-10 Shaw Centre
228208 Singapore
Singapore

G2 LogoMach Alliance LogoISO 27001 Logo
CCPA Logo
GDPR Logo

© 2024 Talon.One GmbH. All rights reserved.