The Dos and Don'ts of Code Reviews

Code reviews are an essential part of a developer's work. That's why we would like to present some guidelines by one of our developers to ensure a healthy and effective review culture.

Code reviews are nowadays a lot more than just a process of checking a program’s source code. Modern development teams have started to extensively leverage them for delivering higher quality products based on well-structured codebases and for sharing knowledge and feedback between their members.

This article will focus on the core principles of what it means to provide 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. 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.

# Using correct indentation (e.g. Python) num = 4 if num > 0: print(num, "is greater than 0.") print(num, "is a number.")
Code language: Python (python)

Developers often spend more time on finding a solution for a problem than on the actual implementation itself, which could sometimes lead to very complicated and confusing code, often caused by badly chosen variable, function and even 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.

// Naming conventions are a must const visitorCount = 14; // Existing code => camelCase is used const currently_online = 1; // New code => snake_case is used instead? => bad
Code language: JavaScript (javascript)

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 to point this out unless specified 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 integral part of every new project’s core setup nowadays, 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 any unnecessary discussions about any subjective preferences and change requests the reviewer might come up with.

Well-formed syntax is one thing, but as a reviewer, we should also keep an eye on possible spelling and grammar mistakes. Nobody wants to find himself in a piece of code which is filled with typos and waste time with figuring out what the previous author’s original intention was in the first place.

Maintainability & correctness

A running app is one part of the equation, but does it actually 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 given code.

Apart from 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 reviewed 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 to spend some time for thoughts on whether the new changes are actually future-proof or not, especially when there are already multiple related pull requests out there which might be conflicting with each other in the future. That being said, it can be very beneficial for us — the reviewer, but also for the rest of the team — to be aware of more areas of the project rather than just the ones we are used to working in and keep an eye 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 really happening, we should either think about talking with the developer directly in order to sort out any uncertainties, but also step up and suggest alternatives or adding 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 deeper into the matter in order to provide the developer with a reasonable description of the things that might be worth rethinking.

What is easily recognizable 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 to another place in order to speed up development is very high. For us reviewers, this is a perfect target to pinpoint.

Once the duplicates 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.

We also must take into account the developers’ wish to create something new and unique. It goes without saying that developers can’t avoid implementing new algorithms or functionality at times, but using native functions, existing libraries and utility tools (e.g. Lodash and Axios) can drastically reduce the amount of self-written code and prevent reinventing the wheel.

// Before code review: search with for-loop const chars = ['A', 'B', 'C', 'D', 'E']; let found = false; for (let i = 0; i < chars.length; i++) { if (chars[i] === 'B') { found = true; break; } }
Code language: JavaScript (javascript)

Being aware of this fact allows us to watch out for similar patterns like the one above, because the benefits of re-using existing functionality are obvious and usually lead to faster, more stable and well-tested code.

// After code review: one-liner by using JS’ some() const chars = ['A', 'B', 'C', 'D', 'E']; const found = chars.some(char => char === 'B');
Code language: JavaScript (javascript)

In short, the developer and we as a reviewer don’t have to care about its internal implementation anymore, on the condition that these tools are incorporated the right way.

Test coverage

If the project we are working on uses 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 further adaptions by setting custom coverage thresholds, etc. This can even include builds being 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 in addition to 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.

// Non-descriptive snapshot test it ('should do stuff', () => { const wrapper = mount(<Component />); expect(wrapper).toMatchSnapshot(); });
Code language: JavaScript (javascript)

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

By considering the aforementioned key aspects of code reviews, we are already on a good way in 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 developer.

Personal preferences

Every one of us is unique, which also applies to our approach for implementing new features. This can often be done in countless different ways.

Therefore, it is very important to put our personal coding and styling preferences aside when reviewing other people’s code and focus on the more important things that are hopefully already defined somewhere in the project’s coding guidelines.

As mentioned earlier, formatting code shouldn’t be the problem of the developer anymore with the tools that are available today.

// Opinionated formatting with Prettier // 1. Prettier: good if (meetsCondition) return true; // 2. Prettier: good if (meetsCondition) { return true; } // 3. Prettier: bad => will format it like 2. if (meetsCondition) { return true; }
Code language: JavaScript (javascript)

These tools can take on most of this work and also support different styles the developers might have already agreed upon upfront.

Even if that’s not the case, there is still no real reason for starting an extensive discussion in one of the pull requests. Instead, it should be discussed within the whole team in an upcoming meeting in order to unblock the developer for new tasks and features.

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, such as 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 be part of the review of a pull request.

// "Destructuring is evil" vs. "We need more destructuring" // No destructuring this.props.onClick(); // Destructuring const { onClick } = this.props; onClick();
Code language: JavaScript (javascript)

Arrow functions () => {} vs. normal function() {} notation:
ECMAScript 2015 (ES6) has introduced new features like Arrow functions that are less verbose than its normal function counterpart, but are additionally implicitly binding the this scope of its surrounding block. Some people swear on them while others use them only when necessary. Both have the right to exist.

React’s class vs. functional components:
According to the React docs, class components are equivalent to functional components. Class components can have more features, though functional components are shorter to write, so there’s no need for us to bother the developer with questions about it.

// Class versus functional components in React // Functional component with the "function" keyword function Welcome(props) { return <h1>Hello, {props.name}</h1>; } // Functional component as an arrow function const Welcome = props => <h1>Hello, {props.name}</h1>; // Class component with the "class" keyword class Welcome extends React.Component { render() { return <h1>Hello, {this.props.name}</h1>; } }
Code language: JavaScript (javascript)

React’s setState() with a function vs. an object:
Using setState() by passing an object and relying on the previous state when necessary is probably the more consistent way to go, but in the absence of guidelines, feel free to use option 1 or 2 if you feel like it.

// Updating the internal component state with setState() in React // Passing an object: good this.setState({ counter: 0 }); // Passing a function: good this.setState(prevState => ({ counter: 0 })); // Passing an object when relying on internal state: very bad this.setState({ counter: this.state.counter + 1 }); // Passing a function when relying on internal state: good this.setState(prevState => ({ counter: prevState.counter + 1 }));
Code language: JavaScript (javascript)

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 these are part of people’s individual styles and therefore should not be addressed in a pull request but rather in the aforementioned way.

“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 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 a lack of respect and empathy for somebody else’s work has no place in reviews. Being a reviewer doesn’t give us more control, nor does it make us any better than the rest — we have only switched roles for a moment to see the code from a different perspective.

As long as there are no specific coding guidelines available, insisting on old paradigms without being able to justify 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 proposed for example, or the “gotos are evil” paradigm, are not really fully applicable to modern programming languages anymore. That’s why we shouldn’t blindly jump onto these statements in a code review, but give context and explain why any given 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 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 reviewer and developer, but also the overall working culture in the whole development team, 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 bossing other people around or giving non-constructive feedback to the developer. No matter how bad the code might be, politeness, honesty, empathy are a must in any discussion.

Here are some cases that have been collected over time and which should be avoided when doing code reviews. The same goes for the person who receives the feedback and might have to comment 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 have faith 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 concrete suggestions or even a possible solution.
  • Avoid bikeshedding:
    Focus on the important parts of the review and the code.

. . . .

Final thoughts

Working alone in a project can definitely have its advantages such as embracing your own coding styles, using the tools of your choice and not having to argue with anyone else why something was done the way it was.

However, without receiving continuous feedback on your work, you might not get to harness your full potential. Working in a team can help you get there, though challenges such as the ones mentioned before can always arise. Code reviews are a perfect instrument for solving some of these issues and promoting knowledge sharing and valuable feedback within and even 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 the feedback is reasonable, objective and constructive, we can all benefit from it!

Want to work with us?

Great digital products and services require detailed research and development. Let’s talk about your needs.

Background_3_overlay50