Martin V.
Martin V.
Oct 16, 2018 · 11 min read

Code reviews are a lot more than just a process of checking a program’s source code nowadays. 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 its 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 for humans 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)

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, not only, but often caused by not so well or wrongly chosen variable, function and even file names.

Moreover, functions might have grown too much over time which why the request for additional refactoring and a split-up into smaller junks could help with increasing readability at the same time.

Naming conventions are a must

When looking at the code sample above, the newly added variable is clearly violating against 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 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 any unnecessary discussions about any subjective preferences and change requests the reviewer might come up with.

Well-formed syntax is the one thing, however, 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 lose time on 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 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 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 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 working in — the bigger picture so to say.

“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 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 been emerged over time, causing us to dig a little bit deeper into the matter in order 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 to another place 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 for creating something new and unique. It goes without saying that a necessity for implementing new algorithms or functionality is obviously 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 in the end prevent another re-invention of the wheel.

Before Code Review: Search with for-loop

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 are obvious and usually means faster, more stable running and already well-tested code.

After Code Review: One-liner by using JS’ some()

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 in the right way.

Test Coverage

If the project we are working with is utilizing testing frameworks, we should also spend some time on 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.

Non-descriptive snapshot test

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, reviewer and 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 our personal coding and styling preferences aside when reviewing other people’s code and focus on the more important things that are hopefully already somewhere defined in the project’s coding guidelines.

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

Opinionated formatting with Prettier

These tools can take over most of this work and moreover, 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 not really a 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 the feature from being shipped.

Once the team has agreed on that the issue is actually one that 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.
“Destructuring is evil” versus “We need more destructuring”
  • Arrow functions () => {}vs. normal function() {} notation:
    ECMAScript 2015 (ES6) has introduced new features like Arrow functions that are less verbose to write 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, on the other side functional components are shorter to write, so no need for us to bother the developer with questions about it.
Class versus functional components in React
  • React’s setState() with a function vs. an object:
    Using setState() by passing in an object and relying on the previous state when necessary is probably the more consistent way to use, but feel free to use variant 1 or 2 whenever you feel like and no guidelines are present.
Updating the internal component state with setState() in React

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 which 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 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 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 literally just 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 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 really 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 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 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 is 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 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 into 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 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 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 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 this feedback is reasonable, objective and constructive, we all can benefit from it!

Parkside

Parkside is a digital design & software engineering company. Routinely entrusted by market leaders of the future.

Martin V.

Written by

Martin V.

Parkside

Parkside

Parkside is a digital design & software engineering company. Routinely entrusted by market leaders of the future.

Welcome to a place where words matter. On Medium, smart voices and original ideas take center stage - with no ads in sight. Watch
Follow all the topics you care about, and we’ll deliver the best stories for you to your homepage and inbox. Explore
Get unlimited access to the best stories on Medium — and support writers while you’re at it. Just $5/month. Upgrade