A proposal for clearer code review

Building a loose feedback framework from personal experience, both as a reviewer and a creator of pull requests.

C o l l a b o r a t i o n. Photo by You X Ventures on Unsplash

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:

1. Create a branch off master
2. Work (fix the bug, build the feature, refactor the legacy code, etc.)
3. 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 space for collaboration.

It can be sometimes a difficult process, and this post will address 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, wether your are 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.

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 downright questioning key architectural concerns. And some others are only raising 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 are 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. But eventually, everything is resolved, and the pull request is merged, but you cannot help but wonder at the time lost by constantly switching context, communicating 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 in these broad categories:

Objective issues

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

• Typos and oversight (mistyped variable names, forgotten commented code)
• Code style (not using camelCase for a variable, formatting errors)
• Mistake (forgot to return the value of a function, forgot to catch errors)

Expressing concern or asking questions

These usually open a conversation. The reviewer does not 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 abstraction
• Proposing the use of a library instead of building a custom solution
• Proposing 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 to notice 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 :

• Putting 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

Those 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. The implementation or not of the suggestion should be up to the pull request’s author. They might include :

• Use of one library over another
• Naming of variables when non legibility issue is raised
• Use of one language feature over another

Complimenting the author

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

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 issue’s type 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 categories previously defined, here is what such a list could look like :

• ✖️ Objective issue
• 🤔 Expressing concern or asking a question
• 🚫 Questioning architectural choice
• 📎 Raising issue unrelated to the current pull request
• 🤷‍♂ Nitpick
• 💯 Complimenting the author

A reviewer would always preface their comment with the appropriate emoji. It could look 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 scanning the comment list and decide 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” 👀

But once we have 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.

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 :

A happy resolution

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

Hopeful outcome

✅Merged

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

• Clearly define the nature of an issue
• Smoothly communicate about issues in the code review process
• Make the process easier and more fun!

My proposal of this approach to my colleagues was met with enthusiasm, and I am looking forward to see if and when we will start reaping benefits from it!