A few days ago, I read "Code Review: The Hope and Sorrow of Programmers". It occurred to me that our team has been carrying out Code Review for two years and we are quite satisfied with the results. I think some of the experiences can be shared and discussed with you.
Why did we implement Code Review? We were faced with a situation of messy code and frequent bugs.
At that time, I felt that I needed to make some changes, hoping to improve the quality of product code and alleviate the difficulties faced by the development team. I personally have a lot of experience in development, and I also hope that this knowledge can be spread within the team.
After various considerations, we believe that implementing Code Review can improve or solve many of the problems we face.

The purpose of this article is not to tell you how to implement Code Review within a team. First of all, I have only implemented it in one company and do not have much experience.
Secondly, the situation of each company and each team is different. The appropriate plan should be selected according to the actual situation of the company or team, and timely adjustments should be made based on the feedback from members to promote the implementation of Code Review.
So, this article introduces how our company implemented Code Review and how we solved the problems we encountered. We hope that our experience can be of some help to you.
The writing was rushed, so if there are omissions or errors, please point them out.

Procedures and Rules

After a simple comparison and trial, we finally adopted the Git Flow + Pull Request (PR) mode for Code Review. (For details of the PR mode, see the Git Workflow Guide: Pull Request Workflow)

Pull Request (PR) simply means that you do not have permission to submit code to a specific repository or branch, and you request someone with permission to merge your submitted code from your repository or branch to the specified repository or branch.
Since PR needs to be confirmed by an authorized person, it is very suitable to do Code Review during this process. Whether to accept or reject depends on the results of the Code Review.
In software that supports PR mode, each PR has a diff interface for the newly added code.
Code reviewers can browse the newly added code requested to be merged online and add comments on the questionable lines of code to implement code review.
Comments can be seen by everyone who has permission to view the repository, and everyone can reply to anyone's comment, a bit like a discussion on a post in a forum.
This mode is a post-review, that is, the code has been submitted to the central repository, and frequent changes during the review process will cause confusion in the historical check-in records.
Of course, Git can solve this problem by changing the history record. However, since it is easy to make mistakes, we generally only implement it on projects with strict requirements, such as basic class libraries.

All the software that supports the PR mode that we know of uses Git as the source code version control tool, so our source code version control tool has also been migrated to Git.
Because Git is so flexible, many Git processes have been created to standardize the use of Git.
Common ones include centralized workflow, feature branch workflow, Gitflow workflow, Forking workflow, and Github workflow.
We made some adjustments to Git Flow, and the adjusted process was named Baza Flow, as defined below.
According to Baza Flow, most of our repositories only define 2 main branches, master and develop. (One exception is that we have a repository with 3 development teams developing at the same time, and 4 main branches are defined. It is still going smoothly. If there are more, merging between main branches will be more complicated.)
The master branch corresponds to the production environment code. All releases for the production environment are sourced from the master branch code. The develop branch corresponds to the local test environment code.
In most cases, QA (testing) only tests the code in the develop branch and the master branch.

Since the developers are all in one team, we do not use repository-based PRs, but branch-based PRs.
We have restricted the operating permissions of the main branch. Only specific people can operate it. The develop branch is for project development leaders and architects, and the master branch is for QA.
Members who have the authority to merge into the main branch will perform the merge according to the agreed rules and will not merge PRs that have not completed the review.
The above point is actually quite important, so we have special agreements for those who have the right to merge, and under what circumstances can the code be merged. (See the explanation of PR below)
The initiator of PR should take the initiative to promote the review of PR. Leader will also pay close attention to the progress of PR review and intervene in time when necessary.

We configure our CI server (what is CI) to only compile specific branches, usually develop and master.
After all the code is merged into the main branch, it will automatically trigger the compilation and release of the local test environment. QA does not need to rely on the code compiled by developers for testing, nor does it need to do these manually, ensuring the independence of developers and testers.
The release of our local test environment includes the release of the database and site. It is fully automatic, and after the release is completed, it becomes a usable product. I can share this part when I have time.

We also used a very important concept in Scrum: Definition of Completion.
That is, we define the completion of a task as: the code is written, self-tested, the submitted PR is reviewed and merged into the main branch.
In other words, the task is considered completed only after all the code has been merged into the main branch, and the code merged into the main branch must undergo Code Review, which is mandatory.

Baza Flow

Current version V0.9

Baza Flow evolved from Git Flow. The development model of Git Flow is shown in the following figure:

Due to the limitations of our hosting software for Pull Requests, we made changes to Git Flow. The changes are:
1. For each major function, we will create a separate feature branch, and project developers will create their own task branches based on this separate feature branch.
For example, for a CS 2 project, the branch created at startup is: master -> develop -> feature/v2.
Developers should create their own task branches based on this large feature branch feature/v2. For example, to create XXXX, you can use a separate branch feature/v2-xxxx.
After completing this task, immediately submit a pull request to the upstream branch (feature/v2). Then create your next task branch from feature/v2-xxxx, such as YYYY edit feature/v2-yyyy.
Please note that the functions merged into the upstream branch must be relatively independent and available. The branch task workload is 0.5-1 working day, and should not exceed 2 working days. If it is not merged upstream after 2 working days, you need to explain to the team.
After the code is reviewed, necessary modifications may be made in the original branch. After the modifications are completed, the code is merged into the upstream branch and the original branch is deleted regularly.
After receiving the notification of successful merge, project team members should merge the upstream major feature branch into their current development branch.
When creating a new task branch after submitting a pull request, be sure to inform relevant colleagues (such as front-end colleagues) and let them continue development on the new branch.

2. For small functions, which are estimated to take 0.5-1 (no more than 2) working days to develop, you can directly create a feature branch based on the develop branch.

3. For bugs encountered in each branch, please create a Bug branch based on that branch.
If there is a corresponding item in the bug tracking management system, please use the ID of the bug tracking management system for naming, such as BAZABUG-1354. For example, the branch name of this bug is bugfix/BAZABUG-1354.
If there is no corresponding item in the defect tracking management system, please briefly describe the modification content in the name, such as "JX 9df2b01 Reference bootstrap css virtual path rewrite to avoid the problem of font not being found", and the branch name can be bugfix/miss-font.
After completing the modification, submit and push it to the central repository and then immediately submit a pull request to the upstream branch.
4. After initiating a pull request, please send the link of the pull request to the code reviewer on IM to notify the other party to review it in time.
2. Execution

We advocate quality first within the team. The development team cannot sacrifice quality for progress, and we have reached a consensus within the team.
Therefore, no matter how urgent the progress is, the Code Review process must be done.
All issues will be raised, but based on the urgency of the progress, the size of the issue, and the cost of change, we will decide whether to solve the problem now or add a TODO and record it in the defect tracking management system to prevent it from being forgotten in the future.
In most cases, we ask for an immediate resolution, even if it means delaying the release.
We are well aware that in most cases, if the problem is not resolved now, it may take an unknown amount of time to be resolved in the future.

We did not encounter much resistance in the process of promoting Code Review within the team.
There are probably two reasons. First, the management understands the various problems encountered before and is eager to make improvements, so they have been supportive from the beginning.
Secondly, most developers feel that they can learn something in the process and have no resistance. When they receive good suggestions, everyone is still very happy.
***, slowly an atmosphere is formed, and the whole team will consciously maintain it.
The following is a picture of the dialogue we reviewed. This guy tried to organize the codes for sending business emails scattered around the system, and used a set of patterns to handle them. He adjusted 3 versions before finalizing the tone, and then modified many details before passing the merger. It took more than a week:

On the surface, Code Review will slow down the progress of the project, but in our more than two years of implementation, we did not feel any delay most of the time.
The reason is that, although the code merging cycle has become longer, the code quality has improved, resulting in fewer bugs and less rework caused by bugs, so the overall progress has not actually been delayed.
I personally think that doing code review for a mature team will actually speed up the overall project progress, but I don’t have any statistical data to support my view. (For software development metrics, students with experience are welcome to let me know)

We have more than one person with the authority to merge in each branch, so that when someone is away on leave, the code can still be merged after being reviewed and approved by other colleagues.

Half a year ago, many new members joined our team. The new colleagues were not very familiar with the specifications, projects, and products. As a result, for a period of time, we encountered the problem of longer PR review cycles.
Combined with some of the problems encountered before, we have summarized a description to reduce the burden of Code Review on developers and speed up the process of PR review.
The instructions are as follows:

Pull Request Description

PR can be submitted only after the task is completed.
PRs should be merged or rejected within one business day.
PRs will be rejected if they have serious problems (including but not limited to architecture issues, security issues, design issues), too many issues, or the task is invalid.
It is strictly forbidden to have multiple tasks in one PR unless they are closely related.
After a PR is submitted, you are only allowed to submit code again for issues found during the review. Unless there are sufficient reasons, it is strictly forbidden to submit code for other tasks in the same PR.

When submitting a PR, there is a description box, and the content will be automatically merged according to the Commit message.
Remember, if a commit contains many commits, please do not use the automatically generated description.
Please describe the problem in brief but clear language (ideally within 3 sentences):

What did you change, what problem did you solve, and code reviewers need to pay attention to changes that have a greater impact.
It is important to note that if changes are made to basic or public components, they must be specifically noted in a separate line.

Principles for inviting auditors:

1. When creating a PR, fill in the "Required Reviewers" column. Only when these people have passed the review, will the merge be allowed.
2. In addition to the "required reviewers", there are some other reviewers. We can invite them as "invited review guests" in the Description.
3. For merging between trunk branches, such as Develop => Master, or Master => Develop, the entire team (development + QA) needs to be listed as "required reviewers".

The list of people who must review is determined by the team and may include:

Team Leader

• Front-end architect (if there are changes to the front-end code) (can be authorized)
• Backend architect (if there are backend code changes) (can be authorized)
• Product Architect
• Someone who is familiar with the problem this PR solves (a colleague who has been in charge of this part of the business before)
• The problem solved by this PR has a greater impact on him (for example, the claimed task depends on the colleague of this PR)

Other reviewers include but are not limited to:

Colleagues who need to know the code changes here but don't have to review them can learn from this PR

Authorization means that, according to the agreement, for changes such as bug fixes or changes with lesser impact, the front-end architect and the back-end architect can authorize a senior developer within the team to review on their behalf.
The merging between trunk branches and the merging of large features require the participation of front-end architects and back-end architects.

The above reviewers focus on different perspectives:
The team leader pays attention to whether you have completed the task, the front-end and back-end architects pay attention to whether it complies with the company's unified architecture, style, and quality, and the product architect pays attention to this PR from the entire product level.
Colleagues who are familiar with the problem can better ensure that the problem is solved and that no new problems are introduced.
The affected colleague can learn about the impact on him in a timely manner.

If the team leader or product architect feels that the number of reviewers invited to the PR is insufficient or excessive, they must adjust them to appropriate personnel, and other colleagues can make suggestions in the comments.

3. Harvest

Our team has gained a lot from implementing Code Review, and the following points can be summarized:

1. Rapidly improved code quality in a short period of time.
There are several reasons for this. People will write their code more carefully knowing that their code will be reviewed.
In theory, the quality of code is determined by the best person in the entire team.
Everyone can also learn from other colleagues' excellent coding during the review process.

2. The number of bugs decreases rapidly.
Unfortunately, we don’t have any statistical comparison data.
I talked to QA, and he gave me data that in one of our new projects, which is released every two weeks, we only find 1 to 2 bugs on average.
This improves the happiness of the entire team, and everyone doesn't have to worry about the pressure so often.

3. Team members will have a more balanced level of familiarity with the project.
New colleagues can quickly become familiar with the team's norms by participating in Code Review.
The code is not understood and familiar to only a few people. Anyone can fix bugs and create new features.
For the company, it avoids personnel risks, and for individuals, it is easier (anyone can help you), and you can choose the tasks you like.

4. Improve the team atmosphere
A lot of communication is required during the review process, and more communication can bring team members closer together.
And regardless of their level, everyone's code must be reviewed, which can create an equal atmosphere within the team.
Every member can review others' code, which can easily motivate them.

Let’s take a look at our data:

We started submitting the first PR on January 17, 2014, and by July 5, 2016, we had sent out a total of 6,944 PRs, of which 6,171 were approved and 739 were rejected. The average number of PRs per day was 11.85, and the most PRs submitted in one day was 55.
These PRs generated a total of 30,040 comments, with an average of 4.32 comments per PR and the most comments per PR being 239.
There were 53 colleagues who participated in the above PR reviews. Each colleague sent 539 comments on average. The most users sent 5311 comments, and the least sent only 1 (a colleague who left the company right after the implementation of Code Review).
It should be noted that only simple questions will be raised through comments. More complex questions, such as those involving architecture, security, etc., will actually be communicated face to face because it is more efficient.

IV. Conclusion

Although Code Review is easier to implement with the right tool support, it does not rely on specific tools, so the previous article did not specify what tools we used except Git.
The reason is that the branch-based PR process relies on creating a large number of branches, and Git makes it very simple to create a branch, so PR mode + Git is a good match.
Before we switched to Git, we also did code reviews, and the process we adopted was to send the commit ID to relevant colleagues for review after submitting the code.
After the review is passed, comments will be made in the defect tracking management system. If the QA colleagues do not see the review passed comments, they will think that the task is not completed and refuse to conduct testing.
Although it was not as direct and convenient as it is now, it was still done.

During the PR review process, common problems encountered by new team members are non-compliance with code standards, which can actually be solved through source code inspection tools. We have been planning this part (( ╯□╰ )), but have not started implementing it.