In the life of a developer, there comes a time when he begins to conduct a code review. As a rule, this is one of the signs of a programmer’s growth: people start to really listen to his opinion and trust more serious tasks. The exact moment when the developer is offered to do the review depends on the structure of the company: there are teams where more or less everything can be done after some time has elapsed, and there are those where the review process rests entirely with the oldest and most experienced colleagues.

At the same time, only few people talk about what principles should be followed to conduct a quality review: what are the main goals of this procedure, what to look for in the first place, how quickly it needs to be carried out.

Therefore, the Google Review Guide looks like a very valuable document, the translation of the first part of which is presented later. Translations of the remaining parts will be released later in separate posts. It is worth noting that this is an adapted translation, not all are translated word-for-word.

Terminology:

CL: “changelist” – a list of code changes sent to the version control system for review. An analogue of Pull Request in GitHub or Merge Request in GitLab.

1.Standards of code review

The main purpose of the review is to improve Google’s code base. All the tools and means used to conduct the review are aimed specifically at achieving this goal.

On the way to achieving the stated goal, we have to make a number of compromises.

First, developers should be able to do their tasks. If you do not accept any changes to the code base, then it will never improve. In the case when the reviewer makes any changes too complicated, the developers simply do not go to do anything in the future.

From the other side, it is the reviewer who is responsible for the quality of the changes to the CL and ensures that the state of the code base does not degrade over time. This is not an easy task, because often the project code is degraded through minor changes over a period of time. This is especially acute when the team is pressured by deadlines and the quality of the little things is sacrificed.

The reviewer is responsible for the code that he is reviewing. He must be sure that the code base remains consistent, supported, and meets all the other principles from “What you need to follow in the review”.

Thus, we come to the following rule, which sets the standard for conducting reviews:

The reviewer should adopt the CL when he improves the overall code base of the project, even if the CL is not perfect.

This is the main principle of conducting a review.

Of course, there are some caveats: if CL adds features to the code that the reviewer does not want to see in the project and does not consider necessary, then he may not accept the changes, even if they improve the code base.

The key aspect here is the realization that there is no perfect code: there is code that can be better. The reviewer should not demand perfection in each separate small piece of CL before accepting changes, he should be able to find balance between improvements in CL as in a code, and importance of those changes which this code brings. Instead of demanding perfection, the reviewer should strive for consistent improvement. CL, which improves the maintainability, readability and comprehensibility of the code should not be delayed in the review for several days or weeks, for the reason that it is not perfect.

At the same time, the reviewer is free to leave comments explaining those points that can be done better, however, are not critical. Mark such comments with a specific prefix, for example, “Nit:”, so that the author knows that this part is not obligatory for execution and its implementation is at his discretion.

Note: this document does not justify changes to the CL that worsen the state of the codebase of the project. The only exception is emergency cases.

1.1 Mentoring

A code review can also carry an educational sense, teaching developers new knowledge about the language, framework, or general principles for writing code. It is always good if you leave a comment that teaches developers something new. In addition, general knowledge improves the codebase. Remember that if your comment has an educational meaning and does not contain critical requirements (according to the standards described in this document), mark it with “Nit:” or write directly in the comment that it is not required to be completed.

1.2 Principles

  •   Facts and data are always more important than opinions and personal preferences.
  •    The style guide is the highest authority. Everything that is not reflected in the style guide should be consistent with other project code. If there is no previous code, then you should accept the author’s style.
  •     Aspects of building programs are never pure questions of writing style or personal preferences. They are based on the rules adopted in the organization / project and should be built taking into account these principles, and not just personal opinion. Sometimes there are several approaches to the implementation of the same functionality. In this case, if the author, based on data or generally accepted engineering practices, can show that the approaches are the same, then the reviewer must accept the author’s choice. Otherwise, you should be guided by the standard principles of building programs.
  •     In a situation not regulated by the rules, the reviewer may ask the author to bring the code to a form consistent with another project code, if this does not worsen the general condition of the code base.

  1.3 Conflict Resolution

In case of a conflict, first of all, you should try to reach a consensus based on the rules described in Guide for authors of CL and Guide for reviewers.

If it is not possible to reach an agreement, then the reviewer and the author should schedule an in-person meeting or a video call: this will be more effective than the war in the comments. In this case, try to record the results of the discussion in the comments to the CL, for future readers.

If the situation is still not resolved, then you need to try to escalate the conflict – you can discuss the issue with the team, find out the opinion of the original author of the code that is being amended, or seek advice from more experienced colleagues. Do not let the CL freeze simply because the reviewer and the author cannot agree.

2.What to look for in a review

Note: Always consider Code Review Standards when considering each of the principles.

2.1 Design (structure)

The most important thing to pay attention to in a review is the overall CL design. How does code interact within the framework of CL? Where are the changes located: in a common code base or in a separate library? How well do they integrate with the rest of the system? Is the time right now for these changes?

2.2 Functionality

Does CL do what it is intended to do? How well thought out are the changes for users? At the same time, “user” means the end user (if changes affect him), as well as developers who will use the code in the future.

It is assumed that the developers test their code well enough and at the time of transmission to the review it works correctly. At the same time, from the point of view of the reviewer, you should think about boundary cases, problems of competitiveness, represent yourself in the place of the end user, and also do not forget about bugs that can be detected simply by reading the code.

There are situations where additional CL checking may be required: for example, when changes affect the user interface and it is difficult to imagine how this will affect the end user through reading the code. In such situations, you can ask the developer to make a small demo with new functionality, if running this code yourself seems difficult.

Another case when you should take a closer look at a review is the presence of parallelism in one form or another in the CL code. The main problems that parallelism can bring are deadlocks and races. These problems are very elusive, therefore, it is necessary that both the reviewer and the developer should be more attentive to this code. (The complexity of the review is also a good reason to avoid competitive models with possible races and deadlocks).

2.3 Complexity 

Check the complexity of the code at all levels of the CL – in separate lines, functions, classes. Too high complexity means that, firstly, you cannot quickly understand how the code works, and secondly, when making changes, bugs will surely appear.

The usual case, as a result of which the code turns out to be too complicated, is when developers write too general code or add functionality that is not needed in the system now. Reviewers should be very vigilant about over-engineering and should convince developers to solve only those tasks that must be solved now, and not concern themselves with what may or may not have to be done in the future. Future problems should be addressed as they become available, at a time when they take on specific traits and get clear requirements.

2.4 Tests

The code passing the review should be covered with tests: require unit, integration or end-to-end tests when conducting a review. In general, tests should be added to the same CL as the code. The exceptions are emergency cases.

Check the correctness of the tests – they do not test themselves, and tests for tests are written extremely rarely, usually people check them.

Will tests fail if code breaks? Will they work correctly if the code changes? Are all tests simple and verify what needs to be checked? Are method checks broken down correctly?

Remember that tests are also code that needs to be supported. Don’t let the tests be too complicated, simply because they are not included in the final release file.

2.5 Names

It must be verified that the developer has chosen the appropriate name. A good name should be complete enough to understand what the component is responsible for, but at the same time, remain easy to read and not be too long.

2.6 Comments

How clear and necessary are the comments left by the developer? Are they written in plain English? Comments are useful when explaining why a given code exists, but are superfluous if they talk about what the code does, although there are exceptions: for example, regular expressions and complex algorithms. If the code itself is not visual, then you need to make it easier. However, most often, comments should explain what the code cannot say: those decisions that are behind the written.

It can be useful to look at comments left before CL: perhaps there is a “TODO”, which can now be removed or advice on how some functionality should be done.

It is important to note that comments are not the same as documentation of classes, modules, and functions. The documentation is needed just to describe what the code does and how to use it.

2.7 Style 

Google has a style guide for all the major and most auxiliary programming languages ​​that we use. Ensure that the CL meets the requirements in the appropriate guides.

If you make a remark that concerns style and this point is not covered in the guide, then mark this requirement as optional (“Nit:”). CL should not be blocked based on personal stylistic preferences.

The developer should not interfere in one CL stylistic changes and correction of functionality. This makes it difficult to understand exactly which functionality has changed. For example, if a developer wants to completely rewrite an entire file by changing its functionality and refactoring, then require two CLs from it – the first with refactoring, and the second with functional changes.

2.8 Documentation

If the CL changes the process of user interaction with the code, for example, how the tests now work or the releases pass, you should also check changes in the documentation, including README, the corresponding g3doc (note – Google’s internal tool for storing documentation) and any links to the generated documents . If the CL removes the code, verify that the corresponding section in the documentation is also deleted. If there is no documentation, then request it.

2.9 Every line

Check every line of code. Some data, such as generated code or a giant data structure, can be read diagonally, but never just flip through code written by a person. Of course, something needs to be studied more closely – you must draw the line for yourself what exactly and how deep. Remember that you should always understand what the code does.

If it is difficult for you to understand what is happening and the review is delayed, then let the developer know about it and, before continuing with the review, wait for clarifications. Google has very strong developers and you are one of them. If you do not understand what is written, then, with a high degree of probability, others will not understand too. Thus, by asking for clarifications, you are helping not only yourself, but also your colleagues who will encounter this code in the future.

If you understand what the code does, but don’t feel competent enough to conduct a review, make sure that among the reviewers there is a person with the appropriate qualifications on this issue. It is especially important to relate to this problem when it comes to security, accessibility, multithreading, localization, etc.

  2.10 Context

Usually, tools for conducting a review show only what the developer has changed and small pieces of code around the changes. But, often, you need to completely review the whole file to understand the context: suppose you see that 4 lines have been added, however, expanding the file you understand that these are lines in a 50-line method and now you need to refactor it.

It is always useful to think about CL in the context of the system as a whole: does the given CL improve the project code or make it less clear, worse tested, and so on?

Do not accept CLs that degrade the codebase. Often, projects become too complex just as small changes are made to them, so it is important to keep track of the complexity in each individual CL.

2.11 Positive points

If you see a job well done in CL, then tell the developer about it: reviewers should not only pay attention to errors, but also reward for quality written code. From the point of view of mentoring, it is much more important to talk about what has been done well than to focus on bad points.

Summary

The most important thing when conducting a review:

  •     The code is well designed
  •     Functionality works correctly
  •     Changes in the user interface require special attention: in addition to the purity of the code, you need to make sure that for the user the changes look as intended
  •     Parallel code is executed safely
  •     The code is not overcomplicated
  •     There is no far-fetched and unnecessary functionality at the moment
  •     Tests are done
  •     The tests are well designed
  •     The correct names are used for files, classes, methods, variables, etc.
  •     Clear and necessary comments are left, for the most part explaining the reasons for the decisions, and not the essence (the essence should be clear from the code itself),
  •     The code is documented (g3doc),
  •     The code matches our style guides.

You should look at each line of the code, take into account the context, be sure that you are improving the state of the code base and encourage successful developer decisions.