

Ways to Make Code Reviews More Effective

| Posted by Trisha Gee on Oct 01, 2016. Estimated reading time: 18 minutes |

Key takeaways

  • There are many, many things a reviewer can look for when performing a code review.  A team should identify what's important to their project, and consistently apply those checks during a review.
  •  Human reviewers are very valuable, so automate everything you can, e.g: formatting, style, checking for common bugs, identifying common security issues, and running automated tests.
  •  When it comes to performance, understanding the system's performance requirements are key to knowing which potential issues to look for during a code review.
  •  Some simple checks by a human code reviewer can dramatically improve an application's security.
  •  A code review should be a collaborative discussion, not a battle. Deciding upfront what is important and what's not can reduce conflict and manage expectations.


It is widely known that performing Code Review in a team helps to increase code quality, share knowledge and responsibility, and ultimately helps build better software and a better team. If you only take a few seconds to search for information about code reviews, you’ll see a lot of articles about the value code reviews bring. There are many ways to do it, too: pull requests on GitHub, or code review in a tool like Upsource from JetBrains. However even when the process is clear and the tooling is right, the big question remains – what is it we should be looking for.

Probably the reason there’s no definitive article on what to be looking for is: there are a lot of different things to consider. And, like any other set of requirements, individual teams will have different priorities for each aspect.

This article aims to outline some of the things a reviewer could be looking out for. Deciding on the priority of each aspect depends on each project’s specifics.

Before we continue, let’s think about the problems people tend to comment on during a code review. It’s easy to argue about formatting, style and naming, and point out missing tests. These are valid things to check, if you want consistent, maintainable code. However, discussing these things during a code review isn’t the best use of time as many of these checks can (and should be) automated.

What can humans spot in a code review that we can’t delegate to a tool?

It turns out there’s a surprisingly large number of things. In the rest of this article we’ll cover a list of a wide range of features, and drill a little deeper into two specific areas: performance and security.


  • How does the new code fit with the overall architecture?
  • Does the code follow SOLID principles,Domain Driven Design and/or other design paradigms the team favors?

Readability & Maintainability

  • Do the names (of fields, variables, parameters, methods and classes) actually reflect the things they represent?
  • Can I understand what the code does by reading it?
  • Can I understand what the tests do?
  • Are the exception error messages understandable?
  • Are confusing sections of code documented, commented, or covered by understandable tests (according to team preference)?


  • Does the code actually do what it was supposed to do? If there are automated tests to ensure correctness of the code, do the tests really test that the code meets the agreed requirements?
  • Does the code look like it contains subtle bugs, such as using the wrong variable for a check, or accidentally using an and instead of an or?

Have you thought about…?

  • Are there regulatory requirements that need to be met?
  • Have user-facing messages been checked for correctness?
  • What are your performance requirements, and have you considered the security implications? These are big areas to cover and key topics, so let’s look at both of them in more detail.


Let’s drill more deeply into the topic of performance, as this is an area that can really benefit from code reviews.

As with all architecture/design areas, the non-functional requirements for the performance of a system should have been set upfront. Whether you’re working on a low-latency trading system which has to respond in nanoseconds or you’re writing a phone app to manage a “To Do” list, you should have some idea of what’s considered “too slow.”

Before deciding on whether we need to undertake code reviews based on performance, we should ask ourselves a few questions about what our requirements are. Although some applications really do need to consider how every millisecond is spent, for most applications there’s limited value spending hours agonizing over optimizations that will save you a few CPU cycles. But there are things a reviewer can check for in order to ensure that the code doesn’t suffer from common avoidable performance pitfalls.

Does this piece of functionality have hard performance requirements?


If code is a response to an issue along the lines of “the login screen is too slow to load,” the original developer needs to have found out what would be a suitable loading time – otherwise how can the reviewer or the author be confident that the speed has been sufficiently improved?

If there are hard requirements, is there a test that proves it meets those? Any performance-critical system should have automated performance tests which ensure that published SLAs (e.g. “all order requests serviced in less than 10 ms”) are met. Without these, you’re relying on your users to inform you of failures to meet your SLAs. Not only is this a poor user experience, but it could lead to avoidable fines and fees.



Calls outside of the service/application are expensive

Any use of systems outside your own application that require a network hop are generally going to cost you much more than a poorly optimized method. Consider:

  • Unnecessary network calls: Like databases, remote services can sometimes be overused, with multiple remote calls being made where a single one might suffice, or where batching or caching might prevent expensive network calls. Again, like databases, be aware that sometimes an abstraction can hide that a method is calling a remote API.
  • Mobile / wearable apps calling the back end too much: This is basically the same as “unnecessary network calls,” but with the added problem that on mobile devices, not only will unnecessary calls to the back-end make your performance worse, but they will also drain the battery faster and possibly even cost the user money.

Using resources efficiently and effectively


Locks are a performance killer and very hard to reason about in a multithreaded environment. Consider patterns like; having only a single thread that writes/changes values while all other threads are free to read; or using lock free algorithms.

  • Could there be a memory leak? In Java, some common causes can be: mutable static fields, using ThreadLocal, and using a ClassLoader.
  • Could memory use grow infinitely? This is not the same as a memory leak – a memory leak is where unused objects cannot be collected by the garbage collector. But any language, even a non-garbage-collected one, can create data structures that grow indefinitely. If, as a reviewer, you see new values constantly being added to a list or map, question if and when the list or map is discarded or trimmed.
  • Does the code close connections/streams? It’s easy to forget to close connections or file/network streams. When you’re reviewing someone else’s code, if a file, network or database connection is in use, make sure it is correctly closed.
  • Are resource pools correctly configured? The optimal configuration for an environment is going to depend on a number of factors, so it’s unlikely that as a reviewer you’ll know immediately if, for example, a database connection pool is correctly sized. But there are a few things you can tell at a glance, for example whether the pool is too small (e.g. sized at one) or too big (millions of threads). If in doubt, the defaults are usually a good start. Code that deviates from default settings should prove the value with some sort of test or calculation.

Warning signs a reviewer can easily spot

Some types of code immediately suggest a potential performance problem. This will depend upon the language and libraries used.



These things are not necessarily going to impact the performance of your system, but since they’re largely related to running in a multithreaded environment, they are related to the topic.

  • Is the code using the correct data structure for a multithreaded environment?
  • Is the code susceptible to race conditions? It’s surprisingly easy to write code that can cause subtle race conditions when used in a multithreaded environment. As a reviewer, look out for get and set combos that are not atomic.
  • Is the code using locks correctly? Related to race conditions, as a reviewer you should be checking that the code being reviewed is not allowing multiple threads to modify values in a way that could clash. The code might need synchronization, locks, or atomic variables to control changes to blocks of code.
  • Is the performance test for the code valuable? It’s easy to write poor micro-benchmarks, for example. Or if the test uses data that’s not at all like production data, it might be giving misleading results.

Code-level optimizations

For most organizations that aren’t building a low-latency application, these optimizations are probably fall under the category of premature optimizations, so understand first if this level of performance is needed.

  • Could the code benefit from lazy loading?

Simple code is performant code

There are some easy things for reviewers of Java code to look for, that will give the JVM a good chance of optimizing your code so that you don’t have to:

  • Small methods and classes
  • Simple logic – no deeply nested ifs or loops

The more readable the code is to a human, the more chance the JIT compiler has of understanding your code enough to optimize it. This should be easy to spot during code review – if the code looks understandable and clean, it also has a good chance of performing well.


How much work you do building a secure, robust system is like anything else on your project – it depends upon the project itself, where it’s running, who’s using it, what data it has access to, etc. Now we’ll highlight some areas you might like to look at when reviewing code.

Automate as much as possible

A surprising number of security checks can be automated, and therefore should not need a human. Security tests don’t necessarily have to be full-blown penetration testing with the whole system up and running; some problems can be found at code-level.


Sometimes "It Depends"

While there are checks that you can feel comfortable answering with a “yes” or “no”, sometimes you want a tool to point out potential problems and then have a human make the decision as to whether this needs to be addressed or not. This is an area where Upsource can really shine. Upsource displays code inspections that a reviewer can use to decide if the code needs to be changed or is acceptable under the current situation.

Understand your Dependencies


You should also try to minimize the number of versions of each library – not always possible if other dependencies are pulling in additional transitive dependencies. But one of the simplest ways to minimize your exposure to security problems in other people’s code (via libraries or services) is to:

  • Use the highest quality library you can

Check if new paths & services need to be authenticated



Does your data need to be encrypted?


Are secrets being managed correctly?


Should the code be logging/auditing behavior? Is it doing so correctly?



Remember to involve experts



Code reviews are an excellent way to not only ensure quality and consistency in your code, but also to share knowledge within or between teams. There are many different aspects of the code and design to consider, even after you’ve automated the fundamental checks. Code review tools like Upsource can help identify some potential problems via inspections that highlight suspicious code and analysis that shows where issues have been fixed, or introduced, in a particular commit. Tools can also ease the process by providing a place to discuss the design and implementation within the code itself, and inviting discussion from reviewers, authors and other interested parties until agreement is reached.

In the end, it takes a team to decide which factors are important to them when it comes to code quality; it takes human expertise to decide which rules to apply to each code review; and everyone involved in the review should be developing and using interpersonal skills such as giving feedback and negotiating compromises in order to ultimately agree on when code is “good enough” to pass the review.

About the Author


Rate this Article

Adoption Stage

Hello stranger!


Get the most out of the InfoQ experience.

Tell us what you think


Email me replies to any of my messages in this thread

Concise and detailed. by Philopator Ptolemy

Great overview and actionable check-list. Thanks!

to the point! by Jitendra Patil

Excellent article.


Email me replies to any of my messages in this thread


Email me replies to any of my messages in this thread

2 Discuss

Login to InfoQ to interact with what matters most to you.

Recover your password...


Follow your favorite topics and editors



More signal, less noise



Stay up-to-date
