Code reviews question and answers

Last month I presented the topic at WeAreDevelopers Conference in Vienna which is dear and close to all the developers. You guessed it right, it's Code Reviews and how it can lead to a quality software. I had a great time presenting it to the large crowd where people from different programming background gathered under the same roof. The overall event was spectacular, exciting and well-organized.

However, in the end, I only had limited time to respond to follow-up questions and unfortunately, due to the event schedule, I could not meet all the people who asked questions through Slido.

Fortunately, organizers of the conference were supportive enough that they sent me the list of questions that were asked after the talk. I am writing this blog post to respond to these questions. If you have follow-up questions on this list, feel free to message me either on Twitter or through an email.

Again, thanks to all who attended this conference. Your support and enthusiasm keep motivating me to do more tech talks like this.

When do you usually do code reviews? Or do you have a certain time as a team where you altogether review open PRs

There is no fixed time for code reviews, but it's a good idea to keep aside a specific amount of time per day to do reviews. For example, I reserve 2 hours of time every day for code reviews. I am not necessarily going to use all of that, but it helps to clear out any pending reviews plus mentoring junior developers about how to do effective code reviews.

Usually a primary code reviewer does review and then tech lead does the meta-review. It's a good idea to reserve and allocate time for code reviews, especially since you might be the best person to give feedback on the specific area of the code.

We use Todoist app to keep track of open MRs and assignee so that we get a good idea of the number of open MRs and which ones to prioritize.
Code review workflow. Force the team to do it weekly or let developer initiate review when he/she wants?

The volume of code review workflow varies with the number of outstanding MRs waiting to get merged. We encourage junior and mid-level developers to get involved in periodic code reviews. They can choose any merge request they want to review in addition to merge request from their immediate team members.

More they review, more questions they can ask and will increase the familiarity with the codebase. With this practice, we are seeing the large increase in the number of team members who initiate code reviews by themselves. In fact, how people perform code reviews is a part of our technical competency model
Is the code review process sustainable in small companies (<5 developers)

I believe this question is directed towards how to conservatively use time and manpower resources. Having gone through the same situation earlier, I can relate to it. Being a small and fast environment, code review is the last thing people want, but on the contrary, this is the best place to initiate code reviews.

When you make new tickets, make sure to include the time it may take to review the code and apply required changes. This is part of the ticket, so it's not something you're wasting time on.

It can be argued that this practice can slow down the development process and consume the resources which might better be spent writing the code, but look at it like this - Smaller companies which get used to bad programming practices and inconsistent code will have difficulty to cope up with scalability as team grows since additional team members might get equally careless with their own form of inconsistency.

But as a small team, if you invest little time in enforcing coding standards and best practices and keep revising them frequently, it can save a lot of time in the future since no matter how many developers you add to the team, they will all have to abide by those standards thus maintaining quality, effectiveness, and consistency of the final code.
What is your opinion of tools such as prettier / linters? How can they affect code reviews?

I am a big fan of them and strongly encourage everyone to include in the build automation system. Tools like linters, when correctly configured with the build system, help a lot and reduce the burden on developers. Especially when you train linter on certain rules, it can save time for code reviewer by relying on these tools to check style violations.

However, it's not always true. I have seen certain examples of complex code where style violations might be present, but these tools are not smart enough to detect all of them. So, integrate them into your build system, but also do the manual inspection whenever you feel it's necessary.
How long a pull request should be open for review?

It depends on various factors such as urgency of the merge, number of people needed to review the code and length of the merge request. Given these factors, ideally, it should never stay in the queue for more than 3 days. If you think MR is sitting there for a day with no preliminary review, you might want to communicate with other code reviewers if they could start a review.

Alternatively, you can also maintain a channel where people can publish their MR and if other team members have time, they can start reviewing it. The reason why we need to be aggressive about code reviews is that stale MR can invite a lot of merge conflicts while other MRs are being merged into the destination branch.
What about if the non-technical CEO in a small company doesn’t care about architecture? Is there anything an architect can do, or just leave?

It's not uncommon for a non-technical CEO to not understand the significance of architecture. However, as a developer, you know exactly why you want and how you implement it.

When it comes to convincing non-technical people, what you want is an engineering manager who can act as a bridge between engineers and managers and convince another side the benefits of best engineering practices in the long term.

Some of the things I brought up during such negotiations are maintainability when our code base gets more complicated, ease of scaling when the team grows, the speed of the application, simplicity of learning a codebase for new developers and how all these factors can produce reliable software, increased customer satisfaction and revenue growth.

And you guessed it right, when you raise question about customer satisfaction and revenue growth, it will certainly catch attention from business people.
What is better - tools that enforce code review (like TFS branch policy) or informal process?

In my experience more you streamline and formalize the code review process, better is the probability that everyone understands it and encouraged to do it as a responsible team member, so I would go with tools that enforce code review.
The participation rate is always low on code reviews. How do you encourage developers to review the code of their team members?

Participation in code review should always be encouraged as the company grows and team matures. In some cases, team members need training on how to do good code reviews. You can also convince them to do more code reviews by explaining the benefits of the process such as learning opportunity, ability to gain inter-team domain knowledge and improvement in the technical expertise.

You can also add the ability of people to do code reviews as one of the major competencies required to get promoted to next level and assign mentors which can regulate what constitutes good code review. In fact, the review process should be part of the regular workflow and integral part of development process. It should never be optional. As long as there is a code being written in the team, code reviews should also be present.
Do you think it's advantageous for testers to get involved in the code review phase?

Having worked in the environment where testers are usually non-technical people, I haven't really had any experience involving testers in code review process, but the idea sounds interesting to me.

If you're adding screenshots or videos to your merge request, it can act as a good reference for testers to verify the feature or fix before it goes in the production. In other cases, testers can also read through inline comments to get understanding of how the underlying feature or bug fix works.
Shorter partial code reviews, or a longer one when full functionality is implemented?

I personally do not recommend longer code reviews. But what constitutes a longer MR is a product of team size and the average size of normal MR specific to the team. I would encourage partial (incremental) code reviews where reviews are given periodically and will be applied towards future incremental commits.

This is applicable and very much beneficial to new team members where it gives them the opportunity to learn codebase better and fix mistakes early in the process in contrast to the case where they submit single large MR and one obvious mistake is repeated consistently through the code. This could prove to be a tedious task for both reviewer and reviewee where same mistake is repeated continuously and reviewee is expected to apply fix in all these places.
How to review source code documentation?

Documentation is highly encouraged and is important in terms of maintainability as codebase increases and logic gets more complicated.

While reviewing source code documentation one has to make sure that comments explain what the following code does with a brief explanation and the reason. We also want to make sure there is no obvious or redundant information added in comments.

Documentation should follow the consistent style (As set by the team style guide), it is updated as underlying code changes and correctly identifies and explains the reasoning behind variables, methods used and logic behind certain decisions whose intent might not be clear to the future developer working on the same piece of code.
How much time a day you spend on code review?

It is very much arbitrary and is based on the volume of merge requests and availability of other reviewers. I reserve 2 hours of time every day for reviews and usually spend around an hour and a half doing code reviews.
What do you think of trunk based development

Trunk-based development might be good for smaller teams,but as team size grows, Git flow will better suit your needs. To me, the trunk based model is monolithic, less flexible and provides less control.

On the other hand, git flow offers a help in the larger team environments with junior devs since it grants tighter control over which branch can be merged. Especially when you have a stable product and large teams, you want to streamline the feature merges and want control over which changes are considered as stable and release candidates.
How you handle lack of competent team members for code review? Also, who reviews the reviewers?

We all have to start somewhere. Usually, people who are good developers and possess strong domain knowledge, also act as solid code reviewers. If team members are not doing a good job of code review, you might want to check their technical competency and willingness to contribute to the team. Afterall, you have to choose code reviewers from your team, so training could be good start where senior developers mentor their junior counterparts on how to do reasonable reviews.

Usually, we have preliminary code reviews where team members who are other than tech lead review the code in the first pass and then the tech lead or architecture take the second and final pass over the merge request. It's not exactly the final pass reviewers have full authority over code quality but other developers can also ask questions about their suggested reasoning and proposed changes to the merge request.
How to decide on style guidelines and coding practices?

Usually, forming a workgroup consisting of selected team members helps a lot. This workgroup may be created to discuss what constitute as good style and best coding practices. You can create style guidelines of your own or use the one already existing on the Github.

Of course, this is not static and will undergo revision and updates based on the team's requirements. In fact, hybrid approach works well for both style guidelines and coding practices where you can directly adopt them from third party sources, but customize them as per your own requirements and convenience.
About MR assignment handling: who should finally merge the MR after a review is done: developer or reviewer?

Usually, a final reviewer (Who might be a tech lead or software architecture) will take a final pass at merge requests. Once the request is approved it's ready to merge.

At my current job, since we have a large team we have train conductors assigned to merge the request into the main branch. Before merging, they will check if merge request and ticket are properly linked, the request passes all the tests, the ticket is moved to appropriate state and approval is granted on the request. Optionally train conductors can also review the code if necessary.

In smaller teams though, it makes sense to let reviewee handle the merge part (After reviewer approves the merge request) since it's him whose name should go on the merge commit.
Do you allow anyone to review or restrict to the dedicated most experienced team members?

Everyone should be allowed to do code reviews since it's not just for pinpointing errors and suggesting improvements but can act as a good learning opportunity for junior developers.

When merge request is submitted, anyone can take a first pass at them or we call it a preliminary code review. However, we have a tech lead or a software architect to take a final pass and approve the request when it looks good.
What if my team lead or colleague is curious/sociopath/not cool guy and it's very hard for me to get him to review my code?

If your colleague is curious, I would be surprised if he's not interested in code reviews. Code review should be a part of development process and be reviewing other team members' code should be highly encouraged. Declining the request to do code reviews is as bad as declining to write a code. which creates a threat to the professional environment and friendly conduct.

At the higher level though, code reviews should be enforced for the greater good of the team and long-term health of the codebase. In fact it could be added as one of the competencies in order to get promoted to the next level. If the person refuses to do code review, does he also refuses other people to take a look at his code? If this is a case, it's certainly a bad sign. In this case, you might want to set up 1:1 meeting with tech lead or upper authority to know the reasoning behind his such behavior and check if this is something he is ready to improve upon
Should a human code reviewee really take a look at formal coding style (eg tabs vs spaces)? Shouldn't tools do this job?

As I mentioned before, it's not strictly a requirement to do a manual inspection, assuming you trust the underlying linter and formatters. However, going by personal experience I have seen cases where linters are not able to catch complex cases of style violations. The manual review can help mitigate cases like these.
How to prevent large code reviews?/Are there any techniques to review large PRs?

We have faced this problem at my current organization and I cannot say we could completely avoid it, but with certain strategies, it can be mitigated.

When you have a ticket to implement a big feature, you might want to divide it into sub-tickets and create merge request for those sub-ticket provided these parts are either sequential or independent of each other. In that way submitting small parts of code won't affect the overall application behavior.

Alternatively, if there is no way you can divide tickets into smaller versions, you can commit the code in increments where feedback from earlier commits can be applied toward future work.
How many people should be involved in code reviews?

At a minimum, we need a preliminary code reviewer who takes the first pass. This could be any member of the team or someone who wants to learn and get familiar with the code. Next, we have an approved code reviewer who could either be a tech lead or software architect and takes a second and final pass and approves the merge request if it looks good or suggested changes have been applied by the reviewee.

In addition to these two, you can have as many members of team as possible looking at the code. But please remember that more the reviewers, more is the possibility of fuelling the discussion and can delay the merge process
How to improve code reviews across different time zones (>5hours)?

It's a good question. We have teams that are based in regions with > 5 hours of time difference. Usually, if code review is urgent enough to get it merged, you can assign designated code reviewers from the same location to review the code assuming you have approved code reviewer present at that location.

If there is anything team members from other region need to get involved in, you may also post it in the code-reviews channel for visibility and it can be picked at the appropriate time. Usually, when you have a team across diverse time-zones and such collaboration is needed, you might want to assign some extra time it takes to review code since feedback and communication cycle can add extra time due to differences in time-zones
How to deal with frequent, small reviews vs avoiding frequent interruption of reviewers?

You can usually set aside fixed time in a day which can be devoted solely toward doing code reviews. If it's really urgent and you're the only person qualified for review and approval, you can make an exception.

In other cases, depending on how busy you are, you can also reassign reviews to other team members. But whichever path you choose to go with, please keep original reviewee and future code reviewer in the loop. Lack of proper communication can result in merge request sitting there without any review.
How much time should developer spend for code review weekly? Time is money for company

I usually reserve 2 hours of time per day for code review. I may or may not use it all, but it's good to have a backup.

Second part implies that code review is the waste of time and thereby money. I disagree with it. You're consuming your time doing code reviews which results in the quality codebase and scalable, maintainable architecture.

By investing time in code reviews, you're saving a ton of time for the company since it's a part of development process. In my experience lot of people fail to see the value of investment in unit tests and code reviews. But in reality, these things offer long-term benefits if you check the progress over the time.
Is there a maximum of changed files in a pull request?

There is no fixed number since it could be just two changed files with hundreds of changes or hundred files with one change in each.

It all depends on the total number of changes in the merge request. However, when the number of changes increase to few hundred, it's time to think about dividing original ticket or doing incremental code reviews.