Code Review: Successful Experience

On the Internet there is a lot of information on the review code:

• How does the review of someone else's code affect the company's culture
• Regulated Security Checks
• abbreviated manuals
• long lists of recommendations
• interpersonal review of code
• why do you need a review code
• proven methods
• more about proven methods
• statistics: how much code review helps to catch bugs
• examples of errors made when reviewing the code

Yes, of course, there are books on this subject. In short, this article outlines how the code review is organized at Palantir. In those organizations whose culture does not accept such a peer review, it may be useful to first read Karl Widgers’s brilliant essay “ Review of a Code with a Human Face” and then try to follow this guide.

This text is taken from recommendations for quality improvement, based on working with Baseline , our tool for quality control of Java code. It covers the following topics:

• Why, what and when are we trying to achieve with code review
  
• Preparing the code for review
  
• Execute Code Review
  
• Code review examples
  

Transferred to Alconost


Comic XKCD 'Code Quality', copied under CC BY-NC 2.5 license

Motivation

We are engaged in the review of the code (RC) to improve the quality of the code and thus want to positively influence the team and corporate culture. For example:

• The authors of the code are motivated because they know that their request for changes will be considered by the review team: the author tries to clean up the rough edges, follow the action plan and generally improve the whole commit. When colleagues recognize your professionalism in writing code, for many programmers this is a reason to be proud.
  
• Knowledge sharing helps the development team in several ways at once:
  

- When RK, it is explicitly stated what part of the functionality was added / changed / deleted, so that in the future the team members could rely on the work already done.

- The author of the code can use a technique or algorithm in which the reviewers themselves will learn something. Overall, a code review helps raise the bar for quality throughout the organization .

- Reviewers may know something about programming techniques or about the code base itself that will help improve or consolidate the changes made; for example, someone can at the same time develop or refine the exact same opportunity.

- Positive contact and communication reinforces social connections between team members.

• Due to the consistency in the code base, the code itself is much easier to read and understand. This makes it easier to prevent bugs and promote collaboration between sedentary and nomadic programmers.
  
• The author of the code is difficult to judge the readability of his own creation, and the reviewer is just easy, because he does not see in what context this code is. Readable code is easier to reuse, it has fewer bugs, and it is more durable.
  
• Random errors (for example, typos), as well as structural errors (for example, non-executable code, errors in logic or algorithms, problems with performance and architecture) are often much simpler to catch a picky reviewer who looks at the code from the side. According to research, even a brief and unofficial review of the code significantly affects the quality of the code and the occurrence of bugs .
  
• Compliance requirements and regulatory frameworks often require mandatory checks. Code review is a great way to avoid common security problems. If the requirements for security in this environment or in relation to this possibility are increased, then a review will be recommended (or even requested) by satraps from regulatory authorities (a good example is the OWASP manual).

What to look for

There is no clear answer to this question, and each development team is required to agree on its own approach. In some teams they prefer to check any changes made to the main branch of the development, while in others they take into account the “threshold of triviality”, beyond which verification is mandatory. In this case, one has to strike a balance between efficient use of the programmer’s time (both the author and the code reviewer) and maintaining the quality of the code. In some strict contexts, it is sometimes necessary to check everything in the code, even the most trivial changes.

The rules for reviewing the code are the same for everyone: even if you are the most senior developer in the team, this does not mean that your code does not require a review. Even if (sometimes it happens) the code is flawless, the review opens up opportunities for mentoring and cooperation, and, at a minimum, helps to look at the code base from different points of view.

When to check

The code review should occur after successful completion of automatic checks (tests, design, other stages of continuous integration), but before the merging of the new code with the main repository branch. Usually we do not resort to formal verification of the total changes made to the code since the last release.

For complex changes that should be added to the main branch of the code as a single block, but so extensive that they cannot be covered in a single check, try a step-by-step review. We create a primary feature / big-feature branch and a number of secondary ones (for example, feature / big-feature-api, feature / big-feature-testing, etc.), each of which encapsulates a subset of common functionality, and each such branch check with the main branch feature / big-feature. After all the secondary branches are merged into feature / big-feature, create a review of the code for infusing the latter into the main branch.

Preparing the code for review

The author of the code is obliged to provide the revision code in a digestible form so as not to take too much time from the reviewers and not to demotivate them:

• Scope and size . Changes should relate to a narrow, well-defined, self-sufficient scope, fully covered by the review. For example, changes may be related to the implementation of some possibility or to the correction of an error. Brief changes are preferable to long ones. If a review is submitted with material that includes changes in more than ~ 5 files, or requires more than 1-2 days to write, or the review itself can take more than 20 minutes, try splitting the material into several self-sufficient fragments and check each one individually. For example, a developer can submit a change that defines an API for a new feature in terms of interfaces and documentation, and then add a second change that describes implementations for these interfaces.
  
• You need to send only complete, independently verified (use diff) and self-tested materials. To save the reviewer's time, test the changes to be sent (ie, run the test suite) and make sure that the code passes all the assemblies, as well as all the tests and all stages of quality check, both locally and on the servers of continuous integration, and then select reviewers .
  
• Refactoring changes should not affect behavior and vice versa; changes affecting behavior should not be coupled with refactoring or reformatting code. There are several good reasons for this:
  

- Changes related to refactoring usually affect multiple lines of code in many files - therefore, this code will not be checked so carefully . Unplanned changes in behavior can leak into the code base, and no one will even notice.

- Major changes associated with refactoring violate the mechanisms of movement, selection and other "magic" associated with the control of source codes. It is very difficult to undo such a change in behavior that was made after the completion of refactoring, which covered the entire repository.

- The expensive working time that a person spends on reviewing code should be spent on checking software logic, and not on disputes about style, syntax, or code formatting. We prefer to solve such questions with the help of automatic tools, in particular, Checkstyle , TSLint , Baseline , Prettier , etc.

Commit messages

The following is an example of a competent commit message, which is maintained in accordance with a widely used standard:

 ( 80 )    ,  ,   .   72 .          ,    —   .   ,       (   );     ,  rebase,    .  -   : « »,   « »   « ».      -,  , ,  git merge  git revert.      .    . 

Try to describe the changes made at the commit, and how exactly these changes are made:

 > .   .    . > .  jcsv-     IntelliJ 

Reviewer Search

Usually the author of a commit looks for one or two reviewers familiar with the code base. Often in this capacity act as a project manager or senior engineer. It is advisable for the project owner to subscribe to his own project in order to receive notifications about new code reviews. With the participation of three or more reviewers, reviewing a code is often unproductive or counterproductive, since different reviewers may suggest mutually contradictory changes. This may indicate a fundamental disagreement about the correct implementation, and such problems should be solved not during the review of the code, but at an extended meeting, in which all interested parties participate in person or in the form of a video conference.

Execute Code Review

A code review is a synchronization point between different team members, so potentially it can stop work. Therefore, review of the code should be operational (take about a few hours, not days), and members and team leaders should be aware of how long it will take to check, and prioritize the time allocated to it accordingly. If it seems to you that you do not have time to complete the review on time, immediately tell the commit author about it so that he can find a replacement for you.

The review should be fairly solid so that the reviewer can explain the change to another developer with sufficient detail. So we will ensure that the details of the code base were known at least to two, and not to one.

You, as a reviewer, must persist in adhering to code quality standards and keep it high. The review of the code is more an art than a science. You can learn this only in practice. An experienced reviewer should first try to let less experienced colleagues change and let them review first. If the author followed the above rules (especially those concerning self-checking and pre-launching of the code), then the reviewer should pay attention to this when reviewing:

purpose

• Does the code allow to achieve the goal set by the author? Any change should have a specific reason (new feature, refactoring, fixing an error, etc.). Does the proposed code really lead us to this goal?
  
• To ask questions. Functions and classes must be justified. If their assignment to the reviewer is not clear, perhaps this means that the code should be rewritten or backed up with comments or tests.
  

Implementation

• Think how you would solve this problem. Otherwise, why? Does your code handle more (borderline) cases? Maybe it is shorter / simpler / cleaner / faster / safer, and functionally not worse? Maybe you caught some deep pattern that is not covered by the existing code?
  
• Do you see the potential for creating useful abstractions? If the code is partially duplicated, it often means that it is possible to extract from it a more abstract or general functional element, which can then be reused in other contexts.
  
• Think like an opponent, however, stay kind. Try to “catch” authors who cut corners or miss specific cases by throwing problem configurations / inputs that break their code.
  
• Think of libraries or ready-made working code. If someone re-implements the already existing functionality - this happens not only because he does not know about the existence of an already ready solution. Sometimes code or functionality is intentionally reinvented - for example, to avoid dependencies. In this case, it should be clearly commented in the code. Is this functionality already provided in any existing library?
  
• Does the change make to the standard patterns? In existing code bases, patterns associated with naming conventions, program logic decomposition, data type definitions, etc. are often traced. It is usually desirable that all changes be implemented in accordance with existing laws.
  
• Are dependencies added during changes that occur during compilation or during execution (especially between subprojects)? We strive to weakly link the code of our products, that is, we try to allow a minimum of dependencies. Changes related to dependencies and the build system should be checked especially carefully.
  

Readability and style

• Think about how you read this code. Are you able to quickly grasp his concepts? Is it reasonably laid, is it easy to keep track of the names of variables and methods? Are you able to trace the code over several files or functions? Has an inconsistent naming been torn you somewhere?
  
• Is the code consistent with well-known programming and style guidelines? Is the code out of the entire project in terms of style, API naming conventions, etc.? As mentioned above, we are trying to settle stylistic disputes with the help of automatic tools.
  
• Does the code have task lists (TODO)? Task lists simply accumulate in the code and eventually turn into ballast. Oblige the author to send a ticket to GitHub Issues or JIRA and attach the task number to TODO. The proposed change should not have a commented out code.
  

Convenience support

• Read the tests. If there are no tests in the code, but they should be there - ask the author to write a few. Truly untested capabilities are rare, but not tested implementations of capabilities — unfortunately, often. Check the tests yourself: do they cover interesting cases? Is it convenient to read them? Does this test reduce overall test coverage? Think about how this code might fail. Standards for the design of tests and the main code are often different, but the standards for tests are also important.
  
• Does a review of this fragment have the risk of breaking the test code, the break-in code or the integration tests? Often, such a review is not done before a commit / merge, but if such cases turn out to be neglected, then everyone will suffer. Pay attention to the following things: deleting test utilities or modes, changing the configuration, changing the layout / structure of the artifact.
  
• Does this change violate backward compatibility? If so, can this change be made to the release at this stage, or should it be postponed for a later release? Violations may include changes to the database or its schemas, changes to public APIs, changes in the task flow at the user level, etc.
  
• Does this code require integration tests? Sometimes code fails as it should be verified using only unit tests, especially if it interacts with external systems or configuration.
  
• Leave feedback on the documentation for this code, comments, and commit messages. Extensive comments clutter the code, and meager commit messages confuse those who later have to work with the code. This is not always the case, but quality comments and commit messages will surely serve you well in time. (Recall when you have seen a gorgeous or just awful comment or commit message.)
  
• Has external documentation been updated? If your project has README, CHANGELOG files or other documentation, is it updated with the changes made? Outdated documentation may be more destructive than none at all, and it will be more expensive to correct errors in the future than to make changes now.
  

Do not forget to praise concise / readable / effective / beautiful code. On the contrary, rejecting or rejecting the code proposed for the review is not rude. If the change is redundant or insignificant - reject it, explaining the reason. If the change seems unacceptable to you because of one or several fatal errors - reject it, again, reasonably. Sometimes the best outcome of a review of a code is “let's do it completely differently” or “let's not do it at all”.

Respect the reviewer. Although, the position of the opponent is sound, you did not realize this opportunity, and you cannot decide everything for it. If it is not possible to come with the reviewer to a common opinion about the code, arrange the consultation in real time and listen to the opinion of a third person.

Security

Ensure that all API endpoints have adequate authorization and authentication that conforms to the rules adopted in the rest of the code base. Look for other common flaws, for example: weak configuration, malicious user input, missing log entries, etc. If in doubt, show the code being reviewed to a security professional.

Comments: brief, polite, incentive

The review should be brief, sustained in a neutral style. Criticize the code, not the author. If something is unclear - ask for clarification, but do not consider that the whole thing is in the ignorance of the author. Avoid possessive pronouns, especially in evaluative judgments: “ my code worked before your changes,” “there is a bug in your method,” etc. Do not chop off: "it will not work at all ", "the result is always wrong."

Try to distinguish between recommendations (eg, “Recommendation: remove the method, so the code will become clearer”), necessary changes (eg, “Add Override ”) and opinions that require clarification or discussion (eg, “Is this behavior true ? If so, please add a comment explaining the logic "). Try putting links or pointers to a detailed description of the problem.

After reviewing the code, indicate how detailed the reaction to your comments you expect from the author, would you like to repeat the review after the changes have been made, for example: “You can safely upload the code to the main branch after these minor corrections” or “Please note my comments and let me know when I can see the code again. ”

Reaction to the review

The review of the code, in particular, is intended to improve the change request proposed by the author; so do not take the recommendations of the reviewer at bayonets; take them seriously, even if you do not agree with them. Reply to any comment, even the usual “ACK” (approved) or “done” (done). Explain why you made this or that decision, why you have certain functions in the code, etc. If you cannot reach a common opinion with the reviewer, arrange a real-time consultation and listen to the opinion of an outside expert.

Corrections should be fixed in the same branch, but in a separate commit. If you merge commits at the review stage, it will be more difficult for the reviewer to track changes.

The policy of merging code in different teams is different. In some companies, the merger of the code is trusted only to the project owner, in others the participant can also do this after his code has been reviewed and approved.

Review code tete-a-tete

As a rule, asynchronous diff-like tools such as Reviewable, Gerrit, or GitHub are great for reviewing code. If we are talking about a complex review or review, the participants of which differ greatly in the level of experience or professionalism, it can be more effective to conduct a review personally - either sitting together in front of a single monitor or projector, or remotely, via video conference or screen sharing tools.

Examples

In the following examples, reviewer comments in listings are entered using

 //R: ... 

Contradictory naming

 class MyClass { private int countTotalPageVisits; //R:    private int uniqueUsersCount; } 

Inconsistent method signatures

 interface MyInterface { /**  {@link Optional#empty},  s   . */ public Optional<String> extractString(String s); /**  null,  {@code s}   . */ //R:    :    // Optional<> public String rewriteString(String s); } 

Library use

 //R:     MapJoiner   Guava String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator); 

Personal preferences

 int dayCount; //R: :   numFoo   fooCount;   ,          

Bugs

 //R:    numIterations+1 –    ? //    –     numIterations? for (int i = 0; i <= numIterations; ++i) { ... } 

Architectural considerations

 otherService.call(); //R: ,      OtherService.    ? 

About the translator

The article is translated in Alconost.

Alconost is engaged in the localization of games , applications and websites in 70 languages. Language translators, linguistic testing, cloud platform with API, continuous localization, 24/7 project managers, any formats of string resources.

We also make advertising and training videos - for websites selling, image, advertising, training, teasers, expliners, trailers for Google Play and the App Store.

Read more