When chaos started in Bitbucket with pull requests in our dev team - we decide to wrote some points to which authors and reviewers could reference.
Intro
The problem
Unfortunately, when fulfilling their planned business goals, the departments of the organisation rarely take into account such a metric as solution code quality. And usually there is no time for normal code review.
However, it should be noted here that any functionality stands on 2 legs (characterised by):
1. It works!! Feature in principle does its job and gives the result. Usually at this point, Jira tasks are bean closed on this and everyone is happy.
2. How does it work?? Is it safe? Fast? Devouring resources? Such a questions asked much less often. Most likely, it come to this either with "diseases" of the developed functionality or with reducing of time for program execution.
So, as a result, there is a serious risk of releasing functionality that has not been asked for "how well is it developed?".
The novelty of the technologies used
The tool for performing code review is the Bitbucket CI system.
You can read the manual and squeeze out all the "juices" on the convenience, speed and quality of code review on the official website.
Practical value
Analysis of errors and recommendations received to improve the proposed code within the pull request leads to an increase in the level of developers competences.
And accordingly, the higher the level of competence, the less risky and more optimal solutions can be proposed by developers within the project needs.
As practice shows, even in conflict situations, valuable experience is formed, which is assimilated by both parties, both the developer and the reviewer.
Goal and tasks
So, the goal of code review is - to get the best code quality and enrich the experience within the team.
Thus, this goal defines 2 specific tasks that need to be completed in order to achieve it:
1. Get the best code quality;
2. Enrich the experience within the team.
These tasks generates 2 chapters of this article that need to be studied and implemented in practice.
1. Get the best code quality
1.1 Check already existing functionality
It is possible that what was brought within the pull request already exists in the project code and the developer simply did not pay attention to the fact that one of the already existing methods of the class can cover the need.
No need for duplicate functionality but with different name.
The author and the reviewer are responsible for duplicating the logic of the project code. We need to check again that the author's functionality is actually not presented in the project.
1.2 Less code is more quality
Viewing too much code is extremely tedious and time-consuming. Therefore, it is definitely better to view small code snippets.
Small pull request -> Quality Check -> Best Code.
Lite pull requests - secret of longevity of author and reviewer.
1 feature = 1 pull request!! If several features are stuffed into one pull request, then the author go and splits into several pull requests in accordance with Jira tasks.
1.3 Code management at commits level
The commits should be written in such a way that it is possible to safely perform rollbacks and cherry-picks without damaging the project code and breaking the heart of a colleague, which has dependent code.
Watch for commits code dependency.
Has a fat commit like "snow avalanche" format, or dependent functionality located in different commits? The author goes to split his code into logical parts and updates the pull request with the normal structure of commits.
1.4 Start from high level interfaces
Concentrate on the main problems like redesigning the class interface or splitting complex functions. Wait for these issues to be fixed before moving on to low-level issues, such as variable naming. Your low-level notes may become irrelevant when the author makes a high-level edit.
Move top-down.
The top-down technique segments the levels of abstraction that you focus on during the review process, helping you and the author to systematically go through the list of changes.
1.5 Only changes review (Diff)
There are incorrect actions when the reviewer discovers something next to the code from the list of changes - and asks the author to fix it.
Look at the "Diff".
If there is something to write about the code behind the change area, then you need to create a separate Jira task and write a comment about the problem with a link to new task.
1.6 Test cases for classes
This is necessary to check the logic correctness of the classes and reduce the risk of errors. It is bad to write dummy tests just to pass coverage % level for quality gates.
Real logic tests.
Must have tests that check the correctness of logic execution. If there are "if" statements, then the test should get into all forks and exceptions.
1.7 Use code examples widely
A good way to improve the perception of the code review by the author is to find an opportunity to give him a gift with an example of the code.
Give solution code examples.
Such approach with code examples acts like nice business-card and as a result forms an senior-level image of the reviewer.
2. Enrich the experience within the team
2.1 Self-checking works wonders
Surprisingly, but the author is able to see most of the errors in his code himself.
Go ahead with self-check!
Before asking colleagues to review, you need to carefully review your code by yourself for compliance with the development principles and also check that code matches to business specs.
2.2 Pull request comments style
Bad example: "Why did you add this? Go and remove it."
Give constructive comments.
It is best to write comments with the link to common development principles, such as SOLID, DRY, KISS, YAGNI etc. Or you should have your own project coding rules.
2.3 Team code traditions
If you see something good, tell the author, especially when it is actually solved the problem outlined in one of your comments in the best possible way.
Formation of development traditions.
It is important to note that author did the right thing and that is exactly what should be done in the future. Look not just where made a mistake and "fell face down to the dirt".
2.4 Dispute resolution and conflict situations
First step is to involve (via @) experienced developers in order to get different opinions by the problem. If a solution is not found in approx 5 comments, a meeting is the best fit. After the meeting there should be no more fights on the pull request.
It is useful to substantiate an opinion.
Important to formalise in the form of constructive points and justify an opinion with links to sources (book, articles) plus benchmarks - in this case, you can do better in resolving the dispute.
Conclusion
The goal stated in the intro "to get the best code quality and enrich the experience within the team" - will definitely be achieved, if you will act based on points in this article.
Anyway, when we review the code, we make a lot of decisions: what to focus on, how to post a review, when to approve the result.
The most effective techniques in each case depend on the personality of the author, in the relationship with the team and the level of culture.
Hone your approach by thinking critically about the results of your review.
When tension arises, take a break and understand without emotions what is really happening.
Sometimes it is better to read additional information on a question from pull request and wait a day than write low-level comments.
Good luck!
And let your code review be like training program that is interesting to the author and reviewer.
If you have what to say about the topic and want to give negative rating - please feel free to write your comments to understand your opinion. Otherwise it is not serious and it is not clear why you vote negatively.
And of course If you like this article, please vote UP - this will support me to write more such posts with real experience!