the rules of a good Code Review for Android

the rules of a good Code Review for Android

Hello, I am Mykhailo Yemelyanov, the head of the Android department at RuStore. A large team of developers works on the site, the project is regularly updated, and the number of new lines of code is constantly increasing.

A bit of history

Over the course of a year, the app store team released an incredible number of Android versions and builds, revisited how the installation process should work several times, and put together a huge number of ready-made SDKs, it seems, under everything that can be imagined. During this time, we formed rules that allowed us to reduce development and testing time and avoid errors in the final product.

In this article, we will tell how we built the Code Review process in RuStore, what problem we solved, share our practices and draw conclusions.

Motivation

For most companies, Code Review issues begin to be resolved when they actually occur. It is often the case that the products and teams at these companies are already quite mature.

We decided to go another way. When RuStore appeared as a product and gained its first million users, the Android team consisted of 10 people. In such a team, the Code Review process was fast: on average, the review of MRs (merge-request) with edits took no more than 3 days. And that was enough. But!
Then the need arose to expand and increase the team by 3 times in a short period. Already at that moment, we understood that the current review process would create big problems for us: the MTM (Mean time merge) MR’ would start to increase, so the TTM (Time To Market) of tasks would also increase.
Considering all these factors, we started to rebuild the Code Review process, solving these problems systematically.

Lifetime of MR in 4Q 2022 (Team: 10 people, 1 reviewer per MR)

Code Review Rules

We started by describing the elementary rules of code verification, so that it would be easier for an engineer to quickly and efficiently conduct a review of any MR of his colleague.

MR can be looked at indefinitely, but it will not cause the code to be of the required quality. And you can watch it right away, especially if your teammate asks a lot, but that doesn’t solve the problem either! Therefore, the best practice is always to answer simple questions honestly:

Criteria

Question

Solution of the task

Does the code solve the described task and only it? Is it listed in the MR description?

Code design

Is the code designed for our architecture requirements? (e.g. models and business logic are highlighted correctly, layering is broken, SRP issues, etc.).

Functionality

Is the behavior of the code expected by the author? Does the code improve the user story?

Complexity

Could the code be written more simply? Will another developer be able to easily understand and use this code if needed in the tasks?

Unit tests

wrote? Do the tests test the logic of the code?

API integration

Is the SDK/API being used correctly during the integration?

Name

Will the chosen names of variables, methods, classes be understood by everyone?

Comments

Are there comments in difficult places? Are they all clear and useful?

Style

Does the code meet our accepted style code?

(usually automated by autoformatting or static analyzer checks)

The Code Review process should not be used to clarify (un)professional relationships or to increase one’s NSV.

How to choose reviewers?

Code Review participants (reviewers) must be familiar with the code being modified. This will allow you to qualitatively achieve the goal of the review itself with minimal time spent. Code Owners are best suited for this role (we talk about how and why you need to appoint code owners here).

How many reviewers are needed?

We use a simple calculation formula that is easy to understand and just as easy to automate:

2R = 1L + 1D

или

3R = 1L + 1D + 1QA

где:

R - reviewer-ы(2-3)

L - техлид (1)

D - инженер (НЕ техлид) (1)

QA - тестировщик (1)

So, we always have 2-3 reviewers participating in each MR. This amount is quite sufficient for our TTM and code quality assurance.

The approver rule, which allows you to configure the minimum required number of reviewers, should be automated.

Redundant rules can be removed, for example, if your task does not require testing and has a label skip-test.

Additional reviewers

If you need the expertise of another colleague to test any changes, please link them in the description or comments under the required piece of code. You can also add him to the reviewers so that you can immediately see that he has checked the code. Make sure that the colleague is aware that he is not required to review the entire MR, but only a specific piece (so that he does not waste time on the entire MR and does not delay the Code Review)!

How much time to lay on the roar?

We calculate the time for Code Review according to the following schedule:

  • The review start time is calculated from the moment the MR is created and reviewers are selected for it (messages are sent to them).

  • Reviewers write comments (Response time ~ 4 hours).

  • The author corrects, then the process is repeated (Reaction time ~ 6 hours).

  • After checking the changes, the reviewers give approval (likes).

  • MR merges into dev or undergoes a separate testing stage.

It is recommended to spend no more than 24 hours on all this, of which no more than 10 hours should be spent on comments, remarks and MR editing (response time = 4 hours, reaction time = 6 hours). The rest of the time is distributed within the personal effective time of the engineer, also laying the lag associated with different time zones of the teams. If the MR refers to the root branch of the feature in classic Git Flow, then the time of testing this branch is added to the time of MR’s infusion.

How to choose code owners (Code Owners)?

Code ownership is a mechanism for dividing code between teams, which allows assigning responsible (Code Owners) separate parts of the code in the project.

As the project and development team grew, it became clear to us that where everyone is responsible for the outcome, ultimately no one is. For example, if in some project module as through the passage the whole team “walks”, then when technical debt appears, it will not be easy to find a responsible person. This prompted us to set up Code Owners in the repository. Now, for any code in the project, a specific team of responsible reviewers is fixed – a known solution, but when building the review process, it plays a key role.

Our multi-module project architecture also helped in this (put pluses if you are interested in learning about it in the future).

Adding Code Owners as reviewers allows you to:

  • improve quality and speed up Code Review;

  • ensure full awareness of all module changes to responsible reviewers;

  • request reviewer selection when creating MR (GitLab displays prompts).

README validation

The project is constantly changing, new modules appear and old ones are removed, but in a hurry, developers often forget to add README.md description files.

For faster immersion in the area of ​​responsibility of the code, we have implemented a validation README.md, which briefly describes: what this module does, what area the code corresponds to, etc.

That’s why we allocated a separate stage to check the correctness of the README.md file in CI.

This is implemented in the form of ValidatorConventionPlugin and task – validateReadme.

In the future, we plan to add other checks to this plugin, so a general task – validate is provided for general validation.

We check locally as follows:

./gradlew validate

or

./gradlew validateRead

For convenience, you can add the –continue flag, then the task will not be interrupted and will throw errors only at the end of checking all modules.

Gradle-task for generating Code Owners from README.md content

A gradle-task – generateCodeowners – was added to the App to generate the ./gitlab/CODEOWNERS file based on the contents of the README.md files.

# [Имя модуля]

# # Owner

[Команда-владелец кода]

# # Description

[Описание модуля]

This task is convenient to use when you create/remove new modules as a result of reengineering, and you only need to correctly fill out the README.md command, run the task and commit the changes.

We also added relevance validation in MR.

What is the result

We have highlighted several important points to consider when organizing the Code Review process, namely:
1. Define criteria and requirements for roaring code.
2. Assign the necessary number of reviewers so that the Code Review process is more efficient and does not take up TTM.
3. Pin those responsible for the code to reduce immersion time in the roar process.
4. Add a convenient description: What does this code do? What is he responsible for?

By following these rules, we were able to conduct Code Review efficiently, even when the team grew 3 times, without slowing down the pace of development.
We are now still managing 3-4 days of MTM per MR including reviews, while we have significantly increased code volume and MR numbers.
It’s worth noting that we’ve stopped getting low-quality code and solutions that happened under the old review process. And this is really a great achievement for such a huge team!

PS Our practices cannot be called universal, in each team the review process can be organized in its own way. We have shared our approach with you, but we do not pretend that it is perfect for everyone 🙂 What practices do you use? We will be happy to read in the comments.
Thank you!

Related posts