Code reviews are more than just a process of checking a program’s source code nowadays. Modern development teams have started leveraging them to consistently deliver higher quality products based on well-structured codebases and to share knowledge and feedback between their members.
This article will focus on the core principles of providing good, meaningful, and constructive code reviews while trying to keep our inner desire for refactoring crusades and personal preferences aside.
The Dos
First things first. Before we start reviewing the code, we should try to get a first glimpse of what the code is about to do and trying to solve, either by looking at the Pull Request’s title and description or by taking a quick look at the specification. This prepares us for the work ahead and lets us better understand the implemented solution from the very beginning.
Readability
As we are more or less familiar with the code’s surroundings, we can finally start looking at the code itself, which hopefully comes in a readable format for humans. Depending on the used language, the syntax is very important, and setting parentheses at the right place or indenting code appropriately can be essential for a program to run, something we should carefully pay attention to — at least to some extent.
Developers often spend more time finding a solution for a problem than on the actual implementation itself, which could sometimes lead to very complicated and confusing code, not only but often caused by misleading or wrongly chosen variable, function, or file names.
Moreover, functions might have grown too much over time which is why the request for additional refactoring and a split-up into smaller junks could help with increasing readability at the same time.
When looking at the code sample above, the newly added variable is clearly violating the existing variable naming by using snake_case instead of camelCase. Even in the case of non-existing conventions, it’s probably worth pointing this out unless discussed differently.
While finding the right names is still one of the developer’s responsibilities, formatting code could become a painful task when not using the right tools or no tools at all. Thankfully, coding and styling guidelines (like the AirBnB JavaScript style guide) as well as the numerous linting and code formatting tools such as ESLint, TSLint, static type-checkers like Flow and TypeScript, and last but not least Prettier, one of the best-opinionated code formatters available today, can be very helpful in that regard.
A healthy set of guidelines and tools should be a fixed part of every new project’s core setup nowadays anyway, as it will
- save valuable time during the development and reviewing process,
- act as an agreement between all parties involved that might have to deal with the code in the future
- and prevent unnecessary discussions about any subjective preferences and change requests the reviewer might come up with.
Well-formed syntax is one thing, however, as a reviewer, we should also keep an eye on possible spelling and grammar mistakes. Nobody wants to find themselves in a piece of code filled with typos and lose time in figuring out what the previous author’s original meaning was in the first place.
Maintainability & Correctness
A running app is one piece of the cake, but does it do what it’s supposed to do? As mentioned at the very beginning, it’s highly recommended to get a rough understanding of what problems the developer is trying to solve with the underlying code.
Next to verifying the code’s correctness and functional aspect, changes to configurations, the processing of sensitive data such as login credentials or payment data, and security-related mechanisms should be faced very critically as they could potentially introduce unwanted data leaks, attack vectors, performance drops, or even worse, a total breakdown of the app.
Furthermore, it’s always worth spending some thought on whether the new changes are future-proof, especially when there are already multiple related Pull Requests out there which might conflict with each other in the future. That being said, it can be very beneficial for us — the reviewer, and the rest of the team, to be aware of more areas of the project rather than just the ones we are used to working on — the bigger picture.
“Don’t comment bad code — rewrite it.”
— Brian W. Kernighan and P. J. Plaugher
Being able to maintain existing code in the future also means that it needs to be self-explanatory. Since this is not always possible, documenting parts of the code doesn’t hurt. In case we find ourselves in a situation during the reviewing process where we are not entirely sure what is happening, we should either think about talking with the developer directly to sort out any uncertainties, but also step up and suggest alternatives or the addition of extra comments.
Optimization & Performance
Another point worth looking into is possible optimization and performance issues that can be found when taking a closer look at the code. Some of them might be obvious to spot, while others could have emerged over time, causing us to dig a little bit deeper into the matter to provide the developer with a reasonable description of the things that might be worth rethinking.
Easy to recognize is redundant code. DRY, a principle in software development, says to not repeat yourself. Especially during prototyping phases or at the beginning of a task, the tendency for copy-pasting code from one place to another in order to speed up development is very high. For us reviewers a perfect target to pinpoint.
Once the duplications have been identified, nested loops, inline generated functions, self-implemented sort algorithms, and other constructs that could increase computational complexity are possibly another sign of weak spots in the code and could have a negative impact on the app’s overall performance.
Also not to underestimate is the developers’ constant inner desire to create something new and unique. It goes without saying that a necessity for implementing new algorithms or functionality is there when new features are requested, however, using native functions, existing libraries, and utility tools (e.g., Lodash and Axios) can drastically reduce the amount of self-written code and eventually prevent another re-invention of the wheel.
Being aware of this fact allows us to watch out for similar patterns like the one above because the benefits when re-using existing functionality usually mean faster, reliable, more stable running, and already well-tested code.
In short, the developer and we as a reviewer don’t have to care about its internal implementation anymore, under the condition that these tools are incorporated correctly.
Test Coverage
If the project we are working with is utilizing testing frameworks, we should also spend some time looking at the hopefully added test cases and verify whether they reasonably cover the newly implemented functionality or not.
Automated coverage reporting tools such as Coveralls or Codecov can help to accomplish this job by giving a quick overview of what has been done so far and which parts of the code would still require some more treatment by setting custom coverage thresholds, etc. Even so far, that builds could, for example, be automatically rejected so that the developer has to take another look at it before handing it over to the reviewer.
Writing test code can often become a very cumbersome and time-consuming activity besides the actual implementation of a feature and could result in not so well written test cases or the excessive usage of very supportive and easy-to-use testing tools like the Snapshot feature in Jest.
With snapshots, one line of test code is usually enough to cover multiple areas of the real code at once, which might have a negative impact on some of the test cases if not well enough documented — definitely a warning signal for us when reviewing code.
The Don’ts
Considering the aforementioned key aspects of code reviews, we are already on a good way to providing superb feedback to the developer. Nevertheless, we should also keep the following things in mind to make the reviewing process enjoyable for both the reviewer and the developer.
Personal Preferences
Every one of us is unique, also when it comes to implementing new features, which — as we all probably know — can be done in dozens of different ways.
Therefore, it is very important to put personal coding and styling preferences aside when reviewing other people’s code and focus on the more important things that are hopefully already defined in the project’s coding guidelines.
As mentioned earlier, formatting code shouldn’t be the developer's problem anymore in the first place with the tools available today.
These tools can take over most of this work and support different styles the developers might have already agreed to upfront so that everyone’s personal preferences can be considered in some way.
And even if that’s not the case, there is still no reason for starting an extensive discussion in one of the Pull Requests. Instead, it should be discussed with the whole team in an upcoming meeting to unblock the developer for new tasks and prevent the feature from being shipped.
Once the team has agreed that the issue needs to be addressed in the future, it should be added to the coding guidelines as soon as possible so that it can be correctly referred to in upcoming reviews.
Enforcing Conventions
The same goes for trying to enforce our own and, in most cases, very subjective conventions when reading through the code of others. As long as the project guidelines do not specify any particular convention and the newly added code looks similar to the existing one, there is no need to request changes for changing variable names because we simply don’t like it, replacing tabs with spaces and vice versa, template strings over normal string concatenation, and other countless things you might be able to think of right now.
- Destructuring vs. no destructuring:
Should objects be destructured before their properties are used? Where do we draw the line between destructuring a single prop versus multiple ones? Questions that shouldn’t make it into the review of a Pull Request.
- Arrow functions
() => {}vs. normalfunction() {}notation:
ECMAScript 2015 (ES6) has introduced new features like Arrow functions that are less verbose to write than its normalfunctioncounterpart, but are additionally implicitly binding thethisscope of its surrounding block. Some people swear on them, while others use them only when necessary. Both have the right to exist. - React’s
classvs. functional components:
According to the React docs, class components are equivalent to functional components. Class components can have more features, on the other side, functional components are shorter to write, so there is no need for us to bother the developer with questions about them.
- React’s
setState()with a function vs. an object:
UsingsetState()by passing in an object and relying on the previous state when necessary is probably the more consistent way to use it, but feel free to use variant 1 or 2 whenever you feel like no guidelines are present.
Although the differences between suggesting a better name and enforcing a new name are very marginal, we need to keep in mind that even trivial things like this are the ones that make us unique and, therefore, should not be addressed in a Pull Request, but rather in the way described before.
“Cargo cult programming” and “Flame war” Material !?#!@#!!
Submitting a review doesn’t automatically mean that our work is done. In many cases, the discussion between the reviewer and developer starts right there and evolves over a Pull Request’s lifetime, depending on the code’s complexity.
We can assume that the developer has most likely tried to give his/her best, which is why acting like an ignorant that is not showing a modicum of respect and has no empathy for somebody else’s work is a total no-go and has no business there. Being a reviewer doesn’t give us more control, nor is it making us any better than the rest — we have just switched roles for a moment to see the code from a different perspective.
As long as no specific coding guidelines are available, insisting on old paradigms without being able to reason about them is inappropriate.
- “Single-Entry Single-Exit” and “gotos are evil” references:
Software engineering has evolved since its early days back in the 1960s, and therefore rules like having “a single function entry/exit”, as Edsger W. Dijkstra was recommending it, for example, or the “gotos are evil” paradigm, are not fully applicable to modern programming languages anymore. That’s why we shouldn’t blindly jump onto these statements in a code review but also give context and explain why this or that part of the code could be improved. - Don’t blindly request the removal of comments:
You might have seen the quote earlier where bad code should not be commented on and instead be rewritten if it’s not self-explanatory. This is true to some extent. However, it’s up to the developer to add additional comments in the end.
Behavior like this can negatively affect the communication between reviewers and developers, but also the overall working culture in your engineering teams, resulting in people being in a bad mood when new reviews have to be requested.
Non-constructive Feedback
A Pull Request is also not the place for commanding other people around, nor is it the place for giving non-constructive feedback to the developer. No matter how bad the code might be, politeness, honesty, empathy, and being nice overall to every participant are a must in any discussion.
Here are some cases that have been collected over time, which should be avoided when doing code reviews. The same goes for the receiving person, who might have to add something in return.
- Don’t state things like “This is a bad variable name!”:
Suggest a better name. - Don’t criticize things in general:
Be more precise and show what changes could be made. - Don’t point out every single occurrence of an issue:
Leave a detailed comment. (depends on the developer’s experience) - Don’t speak in a commanding tone like “Do it like this!”:
Make suggestions, not orders, and put trust in the coder. - Avoid addressing the author directly via “you didn’t”, “your code…”:
Try to use pronouns like “we” and “ours”. - Avoid yes/no questions like “Can we change this?”:
Provide thoughts or even a possible solution at the same time. - Avoid bikeshedding:
Focus on the important parts of the review and the code. - Avoid using emojis like 👎💩:
Use 👍😃. (where appropriate)
Final Thoughts
Working alone on a project can definitely have its advantages, such as embracing your coding styles, using the tools of your choice, and not having to argue with anyone else about why something was done the way you did.
However, without receiving continuous feedback on your work, you might stay below your potential. Working in a team could improve this situation, while challenges due to the aforementioned things could arise. Code reviews are a perfect instrument for solving some of these issues by sharing knowledge and giving/receiving valuable feedback within and across project teams.
That being said, no matter if we accept, reject, or just comment on a Pull Request at the end of the day, as long as this feedback is reasonable, objective, and constructive, we all can benefit from it!
