This article is the first in a six-part series on what code reviews Care about.

Let’s discuss code review. If you spend a few seconds searching for information on code reviews, you’ll find many articles on why code reviews are a good thing (like Atwood’s blog).

You’ll also find plenty of documentation on how to use code review tools (such as our own Upsource).

However, you will rarely find guidelines that tell you what to look for as a code reviewer when reviewing other people’s code.

The reason there is no definitive article telling you what to focus on in code review is probably because there are so many different things to focus on. And, like other requirements (functional or non-functional), different teams have different priorities for each aspect.

Because this is a big topic, the purpose of this article is simply to outline what you need to focus on as a code reviewer when reviewing your code. Prioritizing aspects and checking them over and over again is also a complex topic that could be written in its own right.

What are you looking at when you’re reviewing someone else’s code?

Whether you review your code through a tool like Upsource or a colleague’s presentation, there are some things that are easier to judge. Such as:

  • Format: Where are Spaces and newlines? Are they using tabs or Spaces? How do I lay out the braces?
  • Style: Declare variables/parameters final? Are method variables defined at use time or at the beginning of the method?
  • Naming: Does the naming of fields, constants, variables, parameters, and classes follow standards? Is the name too short?
  • Code coverage: Is there any test code for this code?

It makes sense to check these things — you want to minimize switching between different codes and reduce the cognitive burden, so the more consistent your code looks, the better.

However, using manpower to check these may not be the best use of time and resources on your team, since many of these checks can be automated. There are many tools to ensure that your code is formatted consistently, follows naming standards and uses the final keyword, and can find bugs caused by simple programming errors. For example, you can use IntelliJ IDEA checks from the command line, so you don’t have to require all team members to run checks in their IDES.

What should you focus on?

What kinds of things are humans really good at? What are the things that we find in code review that the inspection tool doesn’t find?

It turns out there are a lot of things. This is certainly not an exhaustive list, and we won’t go into detail on any one item here. However, your team should communicate what code reviews should focus on, and perhaps what you should focus on.

design

  • How does the new code fit into the overall architecture?
  • Does the code follow SOLID principles, domain-driven design, or any other design style your team prefers?
  • What design patterns are used in the new code? Are these design patterns appropriate?
  • If the code base has multiple standards or design styles, is the new code consistent with the current popular one? Does the code move in the right direction, or does it follow old code that will be obsolete?
  • Is the code in the right place? For example, if the code is order-related, are they in Order Services?
  • Does the new code reuse something from the existing code? Does the new code offer something that the existing code can reuse? Does the new code introduce duplication? If so, should it be reconstituted into a more reusable style, or is that acceptable at this stage?
  • Is the code over-engineered? Does the code create reusability that it doesn’t need right now? How does your team balance reuse against YAGNI?

Readability and maintainability

  • Do the names of fields, variables, parameters, methods, and classes really reflect what they represent?
  • Can I understand what the code is doing by reading it?
  • Can I understand what the test code is doing?
  • Do test cases cover good use cases? Do test cases cover normal and abnormal scenarios? Are there any scenarios we haven’t considered yet?
  • Is the error message for the exception easy to understand?
  • Are confusing pieces of code documented, annotated, or covered by easy-to-understand test cases (according to team preference)?

functional

  • Is the code doing what it’s supposed to do? If there are automated tests to ensure that the code is correct, is the test code really being tested against agreed requirements?
  • Does the code seem to contain hidden bugs? Such as using the wrong variable checking, or accidentally using logic and instead of logic or.

Have you considered… ?

  • Are there potential security issues?
  • Are there regulatory needs to be met?
  • Does the new code introduce avoidable performance problems, such as unnecessary database access or remote service access, in areas not covered by automated performance testing?
  • Do authors need to create public documentation, or modify existing help files?
  • Are user interactions checked for correctness?
  • Is there an obvious error that will cause production to stop? Does the code accidentally point to the test database, or is there any hard code that needs to be removed from the real service?

Stay tuned for future articles that will cover these topics in detail.

What to Look for in a Code Review