What are code reviews good for?

Tech blog

My sure-not-yet-complete-and-nevertheless-absolutely-worthy-of-sharing checklist for code reviews

Code reviews improve software quality and distribute knowledge about the software within the team. The more developers look at their colleagues' code, the better we can achieve both goals. For efficient and profitable code reviews, three points are important to me:

  1. The code is judged, not the developer.
  2. The review is based on project standards and guidelines.
  3. The whole team takes responsibility for the code quality.

In my recent projects, we often chose the reviewer at random. Of course, this also means that young developers regularly review code from experienced colleagues. The procedure has advantages for several reasons:

  • The reviewer has to deal intensively with the code and then knows about it as well as the person who wrote the code. Thanks to the shared knowledge, the project is less dependent on individual developers who ultimately want to take a vacation.
  • Younger colleagues learn to program better if they analyze the work of others: They see how another, possibly more experienced colleague uses patterns or implements programming paradigms.
  • Through the review, every developer can see what their colleagues are doing. In this way, the entire team has a better overview of the actual overall architecture and better understands the interrelationships and effects.
  • Four eyes always see more than two. And a fresh pair of eyes often sees the most: Younger colleagues question my solution. They want to understand why I programmed something “like this” - and not “like this”. Your impartiality helps me to reflect on whether another solution is just as compliant with the programming principles. Or even fits better. In any case, the colleague learns something new. And maybe me too.

However, a code review must not be arbitrary. It must be based on the project standards and development guidelines. Younger colleagues in particular often ask me whether there isn't a checklist for the review. That's why I started collecting my best practices and making them available in the project. Some criteria are project-specific, but many are general. My checklist has four categories:

  • High-level architecture and design,
  • Functionality,
  • Readability and maintainability,
  • Nonfunctional requirements.

I do not claim to be exhaustive - on the contrary: The list is still growing, and different aspects are important from project to project.

High level architecture and design

  • Does the code fit into the overall architecture of the project?
  • Have all project specifications, for example naming conventions or style guides, been adhered to?
  • Have new design patterns or data structures been introduced and are they suitable?
  • Is the code in the right place (systems, components, layers ...)?
  • Are technical and business code clearly separated?
  • Is coherent code (in a method, in a class) also on the same level of abstraction? Or are high-level concepts mixed with low-level details?
  • Was something developed that was already there? Or something that should be better covered by a library?
  • Have new dependencies been created that are unnecessary (for example to a second JSON library)?

Functionality

  • Does the code do what is required in the ticket?
  • Does he do more than what is required in the ticket?
  • Was the code over-bred, new German "over-engineered"?
  • Does the code make dangerous assumptions, for example about defaults, value ranges or the availability of systems / information?
  • Are there any subtle bugs, such as a
  • Have the acceptance criteria been implemented as a test?

Readability and maintainability

  • Is it easy to see what the code is doing? Are the tests self-explanatory too?
  • Do the names used (fields, variables, parameters, methods, classes ...) really reflect what they stand for?
  • Have complex codes and unusual special cases been commented on accordingly?
  • Are there any surprising side effects?
  • Do the tests cover the core functionality. For good and bad cases?
  • Do error messages / logs provide enough context information to understand the behavior of the software later?

Nonfunctional requirements

  • Are there any potential security issues with the code? For example, unchecked input values ​​or by violating OWASP rules.
  • Is the code wasteful of system resources? This includes unnecessary database connections, many calls to external APIs or calculations that are made multiple times.
  • Are texts for end users checked for typing and grammatical errors?

Of course, a reviewer can only analyze code according to these key questions if the developer delivers clean code. The minimum requirement for us is: The code is correctly formatted and commented. All tests go through. Static code analysis does not report any obvious errors. and there are no more TODOs, FIXMEs or debug statements in the code. In other words, from the developer's point of view, his ticket is closed.

Is anything missing? I look forward to your suggestion in the comment box below!

Tweet Share Share on LinkedIn