Original: https://medium.com/palantir/code-review-best-practices-19e02780015f author: Robert f. (https://github.com/uschi2000)

Image from https://xkcd.com/1513/

This article talks about the following topics:

  • Why, what, and when are code reviews conducted
  • Prepare code to be reviewed
  • Code review execution
  • Code review example

motivation

The reason code reviews are performed is to improve the quality of your code and inject positive energy into your team and company culture. Such as:

  • Submitters tend to clean up loose ends, merge TODOs, or make general improvements; Once this is done, the submitter expects other reviewers to review the submitted changes. It is the recognition of coding expertise from peers that many developers pride themselves on.

  • Sharing knowledge helps development teams in several ways:

    • A code review clearly communicates functional changes, such as additions, deletions, and changes, to team members for subsequent work
    • Reviewers can learn about certain techniques or algorithms used by submitters. More generally, code reviews help improve quality within an organization
    • The reviewer may have knowledge of programming techniques or a code base that can improve or simplify the submitted code; For example, someone might happen to be developing a similar feature or fixing a similar problem
    • Positive interaction and communication strengthen social bonds among team members

  • Consistency in the code base makes code easy to read, helps prevent bugs, and promotes collaboration between developers

  • The legibility of a snippet of code is difficult to judge for the author who wrote it by hand, and much easier for the reviewer who does not have the full concept of context. Readable code is easier to reuse, less buggy, and less prone to obsolescence

  • Unexpected errors (such as typos) and structural errors (such as invalid code, logical or algorithmic errors, performance, or architectural concerns) are often more likely to be spotted by discerning inspectors. Studies have shown that even brief, informal code reviews can significantly affect code quality and bug frequency

  • A compliant and legal environment is usually subject to review. Code reviews are a good way to avoid common security pitfalls

Review what

There is no one-size-fits-all answer to this question, and every development team should agree on its own. Some teams are happy to review every merge to the Master branch, while others have their own refinement criteria to determine whether a review is needed — a balance between maintaining code quality and using engineers’ time effectively, including authors and reviewers. In some regulatory environments, even minor adjustments may require code review.

Code reviews don’t discriminate: Being the most senior person on the team doesn’t mean your code doesn’t need to be reviewed. Even on the rare occasions when the code is truly flawless, reviews provide team members and partners with the opportunity to at least see the code in the library from a diverse perspective.

When to review

Code review should be done later than the successful completion of automated checking (automated testing, style checking, continuous integration), but before the code is merged into the mainline branch of the repository. There is usually no formal code review for changes accumulated before the last release.

For complex changes that should be consolidated into the mainline branch as a whole, a single code review seems too large, so consider a stacked review model: Create a base branch such as feature/big-feature and some secondary branches (such as feature/ big-feature-API, feature/big-feature-testing, etc.); These secondary branches each encapsulate a functional subset and conduct code reviews independently; Once all secondary branches have been merged into the base branch feature/big-feature, create another code review to merge the latter into the main branch.

Prepare code for review

It is incumbent upon authors to submit code that is easy to review so as not to waste reviewers’ time and initiative:

  • Scope and volume. All changes should have a narrow, well-defined, self-contained scope. For example, a tweak might be implementing a new feature or fixing a bug. A small adjustment is better than a big change. If a code review handles a large number of changes, such as more than five files, more than a day or two of development, or more than 20 minutes to review — consider breaking it up into several self-contained reviews. For example, a developer might submit a change that defines an API for a new feature based on an interface or documentation, and a second change that adds an implementation based on those interfaces.

  • Submit only completed, self-reviewed (with the help of diFF), self-tested code reviews. In order not to waste the reviewer’s time, test the committed changes (that is, run the test suite) and ensure that they pass all builds before assigning the review to them, and ensure that all tests and code quality are checked locally and on the continuous integration server.

  • You can’t change behavior when you refactor; Conversely, adjustments that change behavior should avoid refactoring or formatting code at the same time. The benefits of this are:

    • Refactoring often affects multiple lines of code and multiple files, and these ripples are easily overlooked in the review. Unintentional behavior changes can creep into the code base without anyone noticing.
    • withcherry-pick,rebaseEven minor version control magic will fail in major refactorings. It is cumbersome to undo a commit that introduced a behavior change into the repository as a result of refactoring.
    • Expensive human review time should be spent on program logic, not debating style, syntax, or formatting — which should be resolved with automated tools.

Commit messages

Here’s a good example of a Commit message from the widely cited tbaggery.com/2008/04/19/…

A short and to the point overview is a mandatory part. In some cases, the first line of information is treated as the title, and the rest as the body. Optional more detailed description text. Describe a commit message in imperative sentences (sentences that express commands, requests, admonitions, warnings, prohibitions, etc.), such as:"Fix bug"Rather than"Fixed bug" 或 "Fixes bug". Commit messages generated with git merge or Git Revert also conform to this convention. Empty lines can be used to separate paragraphs. - Lists are also good for paragraphsCopy the code

We should state both what and how for commit:

Dandy recompile dandy add JCSV dependencies to fix IntelliJ compilationCopy the code

Find the censor

Submitters are advised to find one or two reviewers who are familiar with the code base, one of whom should usually be a project lead or senior engineer. With more than two censors, it is easy to be inefficient and even counterproductive because of conflicting opinions. Three monks have no water to drink.

Perform code review

A code review is a process of synchronizing ideas between different team members, with the potential for delays. So code reviews should be quick (in hours rather than days), and team members and leaders need to prioritize reviews and commit to completion time. If you feel that you cannot complete an audit on time, please let the submitter know immediately so that he or she can find another one.

A review should be thorough enough that the reviewer can explain the code changes to other developers at a reasonable level of detail. This ensures that details in the code base can be known to more than one person.

As the reviewer, it is your responsibility to enforce the code specification and ensure that the quality of the code continues to improve. Reviewing code is more art than science. The only way to learn it is to practice it; An experienced reviewer should consider involving other, less experienced reviewers and give them priority.

If the code author has followed the above principles (especially self-review and making sure the code works), here’s a list of things the reviewer should be aware of:

intentions

  • Does the code do what the author intended? There should be a clear reason for each change (new features, refactoring, bug fixes, etc.). Does the submitted code actually accomplish these purposes?

  • Ask questions. The existence of functions and classes should make sense; When reviewers are unclear about what it means, it may mean that the code needs to be rewritten, commented out, or tested.

implementation

  • Think about how you would solve the problem. If so, why? Can your code handle more (marginal) cases? Is your code shorter/easier/cleaner/faster/more secure with the same functionality? Are there any potential unresolved issues in the current code that you have identified?

  • Do you see any potential abstractions available? Repetitive code in particular often means that a more abstract or generic piece of functionality can be extracted and reused across different environments.

  • Think like an opponent and treat people in a friendly manner. Try to “catch” the author’s slack or omission by identifying problems that break the code, such as procedural configuration or data entry problems.

  • Consider libraries or existing production code. When someone re-implements an existing feature, it’s mostly because they don’t know about the existing solution. Sometimes code or functionality is duplicated for the purpose of avoiding dependencies, etc. In such cases, comments should clearly explain the intent.

  • Does the code change follow a standard pattern? Code bases tend to have their own patterns, such as naming conventions, program logic decomposition conventions, data type definitions, and so on.

  • Do the changes add compile-time or run-time dependencies (especially in subprojects)? We want to keep the product loosely coupled, with as few dependencies as possible. Changes to dependencies and build systems should be examined in detail.

Legibility and style

  • Consider your reading experience. Can you grasp relevant concepts in a reasonable amount of time? Is the process sound? Are the names of variables and methods easy to understand? Can you concentrate on multiple files or functions? Have you ever been confused by inconsistent naming?

  • Does the code comply with coding specifications? Is the code consistent with the project in terms of style, API conventions, and so on? Of course, the above mentioned, it is best to use an automated tool to solve the problem, so as not to waste time.

  • Is TODOs still in the code? TODOs simply piling up in code will become obsolete over time, especially if the TODO part is not commented. At the very least, the author should submit the issue to GitHub Issues or JIRA for resolution and put the order number in the TODO notes.

maintainability

  • Read the test. If there are no tests where there should be, let the author write them. The truly untestable features are rare, and in fact the really bad common situation is that some code is not tested at all. Also be careful when reviewing tests: Do they cover interesting and problematic situations? Are they readable? Does the section under review have lower overall coverage? Also important is the style standards of the test code itself, which are often very different from the core business code.

  • Does this test introduce new risks? Such as breaking test code, breaking temporary stacks, or breaking integration tests. The pre-commit/ Merge issues that go along with reviews are often not checked, but they can be a pain in the ass for everyone. Special concerns include changes to test tools and patterns, configuration changes, and artificial changes to the project structure.

  • Does this change break backward compatibility? If so, should you merge the changes now or delay merging until the next release? This can include changes to databases or schemas, changes to public apis, changes to user workflows, and so on.

  • Does this code need integration testing? Sometimes, unit tests alone are not sufficient to validate code, especially when it interacts with an external system or configuration.

  • Code comments, and a Commit Message. Overly verbose comments clutter the code, and overly simple Commit messages confuse newcomers. While not always feasible, the investment in high-quality annotations and commit messages is always worth it.

  • Are external documents updated? If your project maintains README, CHANGELOG, or other documents, are the new changes reflected in those documents? An out-of-date document is worse than no document at all, and it’s much more expensive to fix it then than to update it now.

Don’t forget to praise brief/readable/efficient/elegant code. Conversely, it is not rude to decline or object during a review. If a change is unnecessary or wrong, explain it and reject it. If you feel the change is unacceptable because of one or more major flaws, then do not approve it and explain as well. Sometimes the right conclusion to a code review is “Let’s do this differently” or even “don’t do it at all.”

Respect the partner submitting the audit. While “adversary thinking” works, it’s not your code, and you’re not necessarily thinking it through. If you can’t agree in code, it may be more effective to do so in person or with a third party.

security

Verify that the API side is consistent with the rest of the code base and that appropriate authentication and authentication are performed. Check for other common vulnerabilities, such as weak configuration, malicious user input, missing log events, and so on. If in doubt, seek the help of a security expert.

Note: Simple, friendly and doable

The censor’s notes should be concise and written in human words. Review the code, not as an author. When something is unclear, it is better to ask and figure it out than to assume that it is stupid. Avoid comparisons between people, and it’s even worse to bring comments: “My code worked before you changed it”, “your function is buggy”, etc. Avoid categorical judgments: “This will never work”, “it will always be wrong”.

Try to distinguish between suggestions (e.g., “Suggestions: extract methods for readability”), changes needed (e.g., “add @override”), and clarifications (e.g., “Is this action really appropriate? If so, add a comment to explain the logic “). Also consider providing links, etc. to explain the problem in depth.

When you complete a code review, if you want the author to the extent to which response to your comments, and whether or not they want after the review of the problem to be solved are to review once (for example, “take it easy, after completing the place of a few tips on merger” compared to “please consider my proposal and let me know when to take a look at”).

In response to review

One of the goals of code review is to keep authors on track for improvement; So don’t mope about your censor’s advice, take it seriously even if you don’t think so. Respond to each comment, even if it’s just “known” or “done.” Explain why you made certain decisions, why certain functions exist, etc. If you can’t reach an agreement with your censor, talk offline or seek outside input.

Fixes should be committed to the same branch, but separately. The constant rush of new referrals into the review process has left censors at a loss.

Different teams have different merge strategies: some teams only allow project owners to merge, while others allow contributors to merge code once code reviews have been approved.

Face-to-face code review

For most code reviews, asynchronous diff-based tools such as Reviewable, Gerrit, or GitHub are good choices. Face to face may be more effective when complex changes or reviews are undertaken by parties with different expertise and experience; Whether it’s on the same screen or in front of a projector, or using a remote sharing screen tool, it’s ok.

example

In the following example, the suggestion review comment in the code block begins with //R:… In the form of:

Inconsistent naming

class MyClass {
  private int countTotalPageVisits; //R: The variable names must be consistent
  private int uniqueUsersCount;
}
Copy the code

Inconsistent method signatures

interface MyInterface {
  /** if s cannot be extracted, return {@link Optional#empty}  */
  public Optional<String> extractString(String s);
  When {/ * *@codeReturns null */ if s} cannot be overridden
  //R: should be uniform return value: all with Optional<>
  public String rewriteString(String s);
}
Copy the code

Use the library

//R: Remove this and replace it with Guava's MapJoiner library
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);
Copy the code

Personal taste

int dayCount; //R: Who: I prefer numFoo to fooCount; It's your call, but we should be consistent in this project
Copy the code

Bugs

//R: This is the numIterations+1 iteration. Is this intentional?
// If so, consider changing the meaning of numIterations.
for (int i = 0; i <= numIterations; ++i) {
  ...
}
Copy the code

Architectural concerns

otherService.call(); R: I think we should avoid relying on OtherService. How about we talk about it in person?
Copy the code






–End–






Search fewelife concern public number reprint please indicate the source