Turn: How do we do code review

A few days ago saw "code Review Programmer's Hope and sadness", think of our team to carry out Code Review also has 2 years, the results are relatively satisfied, some experience should be able to share with you, explore.
Why should we implement code review? We were faced with code clutter and bug-frequency situations.
At the time I felt that there was a need to change, to improve the quality of the product code and to improve the difficulties faced by the development team. And I personally have a lot of experience in development, I also hope that this knowledge can be spread within the team.
After various considerations, we concluded that the implementation of code review could 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 because I have personally implemented it in one company and I do not have much experience.
Second, each company, each team situation is not the same, should be based on the actual situation of the company or the team to choose the appropriate program, and according to the feedback of members to adjust in a timely manner to promote the implementation of code review.
So, this article is to introduce our company is how to implement code review, how we solve the problem that we encounter, hope our experience can bring some help to everybody.
If there is any omission or error, please correct the wording.

I. Processes and Rules

After a simple comparison and trial, we finally used the Git flow+pull Request (PR) mode to do code Review. (PR mode details can be found in the Git workflow Guide: Pull request Workflow)

Pull request (PR) simply means that you do not have permission to submit code to a particular warehouse or branch, and you ask someone with permission to merge your submitted code from your warehouse or branch into the specified warehouse or branch.
Because the PR needs to have the permission of the person to confirm, so is very suitable in this process to do code Review, whether accept or reject depends on the result of code Review.
In support of the PR mode of the software, each PR has a new code of comparison (diff) interface.
Code Review can be implemented in this way by reviewing the new code for the request merge online and adding comments to the line of code in question.
Comments can be seen by anyone who has permission to view the warehouse, and everyone can reply to any comment, a bit like a discussion on a post in the forum.
This mode is audited after the event, that is, the code has been submitted to the Central warehouse, the frequent changes during the review process will result in the confusion of historical check-in records.
Of course, git can use change history to solve this problem, because it is easy to misuse, we generally only in the basic class library, such as the more stringent requirements of the implementation of the project.

We've learned that software that supports PR mode uses GIT as the source version control tool, so our source code versioning tools are also migrating to Git.
Because git is too flexible, many git processes have been created to standardize git's use.
Common centralized workflows, functional branching workflows, gitflow workflows, forking workflows, and GitHub workflows.
We've made some tweaks to git flow, and the adjusted process is named Baza flow, as defined in the following article.
According to Baza Flow, most of our warehouses define only 2 trunk branches, master and develop. (with the exception, we have a warehouse with 3 development teams at the same time to develop, define the 4 trunk branches, is still relatively smooth, it is more difficult to estimate the main branch of the merger between the more cumbersome. )
Master corresponds to the production environment Code, and all publishing sources for the production environment are the code for the Master branch. The develop corresponds to the code of the local test environment.
In most cases, QA (testing) tests only the code of the Develop branch and the master branch.

Since the developers are all in one team, we do not use the warehouse-based PR, which is based on the branch of the PR.
We limit the operation of the trunk branch, only the specific personnel can operate, develop branch is the project development leader and architect, Master branch is QA.
Members with permission to merge to the Trunk branch perform the merge in accordance with the agreed rules and do not merge the PR without completing the audit.
The above point is really important, so we will have a privileged merger of the people have a special agreement, under what circumstances to merge the code. (See post-article PR instructions)
PR promoter to actively promote the PR audit, leader will also pay close attention to the progress of PR audit, when necessary timely intervention.

We configured the CI server (what is CI) to compile only specific branches, usually the develop and master branches.
When all the code is merged into the Trunk branch, the compilation and the local test environment are automatically triggered, and QA does not have to rely on the developer's compiled code to test it, nor does it have to do it by hand, ensuring that developers and testers are independent of each other.
Our local test environment release contains the database and the site of the release, fully Automatic, released after the completion is a usable product, there is time this part can also share.

We also used a very important concept in scrum: to complete the definition.
We're the one who set us up. The completion of a task is defined as: The code is written, after self-test, the submitted PR is audited and merged into the trunk branch.
That is, all the code is merged into the trunk branch and the task is completed, and the merge into the trunk branch must go through code Review, which is mandatory.

Baza Flow

Current version V0.9

Baza flow evolved from git flow, as shown in the development model for GIT flow:

Because of our hosting software's restrictions on pull request, we've made changes to git flow, where we've changed:
1, each big function we create a separate feature branch, and the project developer creates its own branch of the task based on this separate feature branch.
For example, for CS 2 projects, the branch was created when it was started: master, develop, feature/v2.
Developers should create their own task branches based on this large feature branching feature/v2, such as creating XXXX, which can be feature/v2-xxxx with a separate branch.
After completing this task, submit the pull request to the upstream branch (FEATURE/V2) immediately. Then create your next task branch from Feature/v2-xxxx, such as yyyy edit FEATURE/V2-YYYY.
Note that the functions that are merged into the upstream branch must be relatively independent and available, with a branch task workload of 0.5-1 working days, not exceeding 2 business days, and not merging upstream for more than 2 business days, which needs to be explained to the team.
After the code has been review, it may make necessary changes, modify the original branch, modify the code to merge into the upstream branch, the original branch will be deleted periodically .
When a project team member receives notification of a successful merge, it merges the upstream large feature branch down into its current development branch.
When you create a new task branch after submitting a pull request, be sure to check with colleagues (such as front-end colleagues) to continue developing on the new branch.

2, for the small function, is expected in 0.5-1 (not more than 2) working day workload of the development task, directly based on the develop branch to create a feature branch.

3. Create a bug branch based on this branch if you encounter a bug in each branch.
If there is a corresponding item on the Defect tracking management system, name it using the ID of the defect tracking management system, such as BAZABUG-1354, for example, the branch name of the bug is bugfix/bazabug-1354.
If there is no corresponding item on the Defect tracking management system, name please brief description of the changes, such as "JX 9DF2B01 reference bootstrap CSS virtual path rewrite, to avoid the font cannot find problems", the branch name can be bugfix/miss-font.
Commit and push to the central warehouse and submit the pull request to the upstream branch immediately after the modification is complete.
4. After the pull request is initiated, please send the link on the request to the code reviewer on im to notify the other party to review it in a timely manner.

Second, the implementation

We promote quality priorities within the team, and the development team cannot sacrifice quality for progress and reach consensus within the team.
So, no matter how tight the schedule is, the Code review process will certainly do.
All the questions will be raised, but will be based on the urgency of the progress, as well as the size of the problem, change the cost, determine whether the problem is now resolved, or add a Todo, and recorded in the Defect tracking management system, in case later forgotten.
In most cases, we will ask for an immediate resolution, even if it causes a delay in the release.
We know that, in fact, most of the time, now do not solve the problem, we do not know se years to solve.

We did not encounter too much resistance in the process of implementing code review within the team.
The reason is about two points, first of all management to understand the problems encountered before, but also eager to improve, so from the beginning is the attitude of support.
Secondly, the vast majority of developers feel that in the process can learn things themselves, and no conflict, met very good advice when everyone is still very happy.
Finally, slowly formed a kind of atmosphere, the whole team will consciously maintain it.
Attached to our audit of the dialogue chart, the children's shoes to try to scattered throughout the system to send business mail code to do a collation, with a set of patterns to deal with, adjusted the 3 version before the tone, and then modified a lot of details to pass the merger, about one weeks before and after the time:

On the face of it, code review slows down the project, but most of the time we haven't felt a delay during our 2 years of execution.
The reason is that while the code merge cycle is getting longer, the overall progress has not slowed down because the code quality has improved, resulting in fewer bugs and fewer rework problems due to bugs.
I personally think that a mature team actually doing code review will speed up the overall project progress, but there is no statistical data on hand to support my point of view. (For software development measures, welcome to have the experience of students tell me)

We have more than one person in each branch who has permission to merge, so that when someone leaves the office, the code can still be merged after the other colleague approves it.

Six months ago, our team joined a lot of new members, just joined the new colleagues on the specification, project, product familiarity is not high, resulting in a period of time, we encountered a PR audit cycle longer issues.
In addition to some of the previous problems, we summed up a note, the purpose is to reduce the code review on the developer's work, speed up the PR audit process.
The description is as follows:

Description of Pull Request

The task is completed before the PR can be submitted.
The PR should be merged or rejected within one business day.
PR is rejected if there are serious problems (including but not limited to architectural issues, security issues, design issues), too many problems, or if the task is invalid.
It is forbidden to have multiple tasks in a PR unless they are closely related.
The PR submission only allows the code to be re-submitted for review discovery, unless there is sufficient reason to prohibit the submission of other tasks in the same PR.

When you submit a PR there is a description box, and the content is automatically merged according to the message of the commit.
Remember, do not use auto-generated descriptions if the content of one commit contains many commits.
Describe the language that is short but sufficient to explain the problem (ideally controlled in 3 words):

What you've changed, what you've solved, and people who need code review to keep an eye on the big changes.
In particular, it is important to note that if you make changes to the underlying and public components, you must specify a separate line.

Review the principle of inviting people to invite:

1. In the creation of PR, reviewers (auditor) in the column of the main fill in the "Required auditor." Only these people are approved for approval before they are allowed to merge.
2. In addition to the "required auditor", there are some other auditors, we can be in the description as "invited to review guests" @ Come in.
3. Merging between trunk branches, such as develop + master, or master = Develop, requires that the entire team (development +qa) be listed as a "required approver".

The list of people that must be audited is determined by the team and may include the following candidates:

• Team Leader
• Front-end architect (if you have front-end code churn) (can authorize)
• Backend architect (if you have back-end code churn) (can authorize)
• Product Architects
• Familiar with this PR solution (previously the colleague who has been responsible for this part of the business)
• This PR resolution of the problem to him is relatively large (such as the claimed task relies on this PR colleague)

Other auditors, including but not limited to:

People who need to be aware of the code changes here but do not need to be audited by their colleagues
Colleagues who can learn from this PR

Can be authorized to refer to, according to the Convention, bug fixes and other changes, or affect minor changes, front-end architects and back-end architects can authorize a team of senior developers, by this senior developer on their behalf to review.
Merging between trunk branches, merging large feature, front-end architects and back-end architects need to be involved.

The reviewer's focus is different:
Team leader focus on whether you have completed the task, the front-end architect is concerned with the company's unified architecture, style, quality, product architects from the entire product level to focus on this PR.
Colleagues who are familiar with this issue can better ensure that the problem is resolved and that no new issues are introduced.
The affected colleague can be in time to understand the impact he has suffered.

If a team leader or product architect feels that the PR invitation is insufficient or excessive, it must be adjusted to the right person, and other colleagues can suggest it in the comments.

Third, the Harvest

Our team implemented code review harvest, summed up probably the following points:

1, quickly improve the code quality in the short term.
For several reasons, you know that your code will be reviewed and written more carefully.
Theoretically, the quality of code is determined by the best person in the whole team.
We can also learn from other colleagues in the process of review excellent coding.

2, the number of bugs quickly reduced.
But we do not have statistical comparison of statistics, it is more regrettable.
I talked to QA, and the data he gave me was a big release every 2 weeks in one of our new projects, and on average only one bug was found.
This improves the happiness of the whole team, and you don't have to be burn often.

3, the team members of the project will be more familiar with the degree of balance.
New colleagues can quickly become familiar with the team's specifications by participating in code review.
Code will not only be known to the individual, familiar with the bug who can change, new features who can do.
For the company to avoid the risk of personnel, it is easier for individuals (who can help you), you can choose the task you like to do.

4, improve the atmosphere of the team
The process of review will require a lot of communication, and more communication can bring the team members closer.
And regardless of level, everyone's code is to pass review, can create an equal atmosphere within the team.
Each member can review other people's code, which is very easy to motivate their enthusiasm.

Light Up our data:

We started from January 17, 2014 the first PR of the submission, to July 5, 2016 issued a total of 6,944 PR, of which 6,171 passed, 739 rejected. Daily average of 11.85 PR, up to a day to mention 55 Pr.
The PR produced a total of 30,040 reviews, with an average of 4.32 reviews per PR, and up to one PR with 239 reviews.
A total of 53 colleagues who participated in the PR comments, 539 comments per colleague, the largest number of users issued 5,311 comments, at least 1 (just implemented code review on the separation of colleagues).
It needs to be explained that only simple questions are raised by comments. More complex, such as related to the structure, security and other aspects of the problem, in fact, will face to the communication, because it is more efficient.

Iv. Summary

While it is easier to implement code Review with the right tooling support, it does not rely on specific tools in itself, so the previous article does not specify what tools we use, except Git.
The reason is that the branch-based PR process relies heavily on creating branches, and Git creates a branch that is very simple, so PR mode +git is a good match.
Before switching to git, we also did code Review, using the process of submitting the code and sending the ID of the commit to the relevant colleague for review.
After the audit will be in the defect tracking management system in the comments, QA colleagues did not see the review approved by the assessment that the task is not completed, refused to test.
It's not as straightforward as it is now, but it still does.

PR audit process, the new team members of the common problem is not conforming to code specifications, in fact, can be resolved through the Source Code inspection tool, which we have been in the planning ((╯-╰)), and did not start implementation.

Turn: How do we do code review