Menu Menu
Code Review — Why and How
Blog

Code Review — Why and How

MOTIVATION

What Is Code Review?

When a developer finishes working on an issue, another developer reviews the code and considers questions like:

  1. Are there any obvious logic errors in the code?
  2. Considering the requirements, are all cases fully implemented?
  3. Does the new code conform to the existing style guidelines?


Code reviews should integrate with a team’s existing process. Most modern software developer teams are implementing features on a task level. This means code review should be initiated after all the code has been written for the specific task (user story or bug fix). This prevents poor coding decisions from polluting the main line of development.

What Are the Benefits of Code Review?

  • Work is decentralized across the team — code reviews help facilitate knowledge sharing across the codebase and the team, meaning anyone from the team should be able to take over any of the Jira ready tickets.
  • The team makes better estimates as product knowledge is spread across the team.
  • Code reviews enable time off — nobody likes to be the sole point of contact on a piece of code. Likewise, nobody wants to dive into a critical piece of code they didn’t write – especially during a production emergency.
  • Code reviews are used to mentor newer engineers — when new members join the team more seasoned engineers onboard the newer members. Code review helps facilitate conversations about the codebase. Often, teams have hidden knowledge within the code that surfaces during code review. Newer members, with fresh eyes, discover gnarly, time-plagued areas of the code base that need a new perspective.

Note: Code review is not just a senior team member reviewing a junior team member’s code. Code review should happen across the team in every direction. Knowledge knows no bounds!

But Code Reviews Take Time!

Sure, they take time. But that time isn't wasted — far from it!
Here are a few ways to optimize it:

  • Requiring code review before merging upstream ensures that no code gets in unreviewed, meaning questionable things and potential bugs get caught before they have a chance to make a lasting impact on your application.
  • To avoid bottlenecks, especially in big teams, the team shouldn’t wait for all developers to do the code review over a particular feature (except in cases when some critical changes should be made ). The team can agree on an optimal number of developers that have to be included and whose approval is mandatory for merging, each additional reviewer is a bonus.
    As agile suggests, this is one of the things that can be adjusted over time.
  • Use peer pressure to your advantage — when developers know their code will be reviewed by a teammate, they make an extra effort to ensure that they design the code in the best possible way so that the review would go smoothly.

PROPOSED SOLUTION

Git is the most commonly used version control system. Git tracks the changes you make to files, so you have a record of what has been done, and that you can revert to specific versions if you ever need to. Git also makes collaboration easier, allowing changes by multiple people all to be merged into one source.

Collaboration between developers

Git has become a standard, as we are able to work collectively with the source code and push our changes back to the team repository as soon as we are done with our work. This does not mean, however, that there are no unique challenges when working in Git. To solve them, a Gitflow branching model was created. Simply put, it is designed in the following way:

The main branch is a highly stable branch that is always production-ready and contains the last release version of the source code in the production.
The develop branch is derived from the main branch and it serves as a branch for the integration of different features planned for an upcoming release. This branch may or may not be as stable as the main branch. It is where developers collaborate and merge feature branches.

Both of these are protected so that code that is not ready isn’t pushed by mistake.

Besides those two primary branches, there are other branches in the workflow:

  1. Feature — is derived from the develop branch and it is used to develop features. This branch is merged back to develop branch with the Merge request after a feature is complete.
    Merge requests are created in the Git hosting platform we use. There we add reviewers who can then do the code review — leave comments, questions and suggestions. The author then addresses them by either changing the code or continuing the discussion in the comments. When the reviewer is satisfied, they approve the MR. Open MRs should be checked ASAP when they are opened so we are not blocking the tickets for too long.
  2. Release — is also derived from the develop, and it merges back into the develop and the main branches after the completion of a release. It’s created and used when features are completed and finalized for a versioned release. It should contain the version number in the branch name.
    This approach enables the rest of the team to work on features for the next release while one developer can separately work on this release’s stabilization.
    Once this branch is merged to the main branch, we delete the branch and tag the main branch with that version number for tracking purposes.
  3. Hotfix — is derived from the main branch and is used to fix a bug in the main branch that was identified after the release. When this is completed, it is merged back to develop and main branches.

We do this because there is a potential problem you might face when branching off from the develop — some developers might have already started work for the upcoming release while you are still in the middle of the current release. Your release would contain the next version of features, which are not finalized and tested, but you only need to provide bug fixes for the current version. Instead of branching off from develop, you can branch off from the main, as that branch contains only the current version of the code in production. This way, branching off from the main branch will not affect the production nor the development version of the product.

GitFlow example graph

To have a clean Git history that helps us understand the decision process behind some code later on, it is important to have structured commit messages. Briefly, a commit message should start with the Jira/Trello ticket identification, space, and then the description of work being committed giving some context to the reviewers and your future self to easily understand the changes, if you come back to it in the future. There are a lot of articles online discussing the topic on how to write good commit messages.


If you like to create a lot of small commits, but still want clean history behind you, you can squash multiple commits into a single one. It can be done locally on your feature branch or you could use the option in the hosting platform to squash commits before merging your merge request into the develop branch. This approach leaves a cleaner history but loses parts of the development progress history and is up to the teams to decide what is more important to them.


If you are using SourceTree as a Git client on your machine, there are GitFlow shortcuts to create a new feature, release, or hotfix branches. Of course, there are simple git commands that can be executed in the terminal.

It would take you some time to become familiar with this approach if you have never used it before, but the future product development and the team can greatly benefit from it.

WHEN NOT TO USE GITFLOW

It is proven that GitFlow is a good choice for the teams who do not have a fast release cycle — the application is released, for example, once per week or less often. If your team releases once per day or even more frequently than that, there are simpler Git branching models that could suit your purposes better.

Github Flow is a continuous deployment environment which does not include the concept of “release”. Every time the feature is finished on a feature branch, the merge request is created to the master branch and when it’s merged it can and should be deployed. This is a lightweight flow suitable for projects which have good test coverage, which would be executed before the merge to master.

Ship/Show/Ask is a branching strategy that combines the features of Merge Requests with the ability to keep shipping changes. Every time the change to the codebase is made, the developer chooses one of three options: ship, show, or ask.

Ship: merge into master without review, using all the usual Continuous Integration techniques. This works great when adding a feature using an established pattern, fixing small bugs, or improving the code based on a colleague’s feedback.

Show: open a merge request for review, but merge into master without waiting for the approval. Changes are live quickly while still creating space for feedback and conversation. This works great when you want feedback on your code, want to show the new approach you used or you generally changed something you want to share with the team.

Ask: open a merge request for discussion before merging. It’s the same approach used in GitFlow to open the MR and wait for feedback. This works great when you are not sure what the team thinks of the new approach you tried out or you need help to improve the implementation.

Note: If you think about how your team works, you’ll notice you’re doing some balance of ship/show/ask, where most teams fall into one of two brackets: “Mostly Ship” or “Mostly Ask”.
Whatever approach you choose, you’ll notice that the code review is an important part of the development process and the Git strategy you use is only a tool to make your job easier.

FUTURE POINTS

This type of workflow is the first step to enable the team to introduce a Continuous Integration and Continuous Delivery system to their project. CI/CD automatically builds the application when any new feature branch is merged to the develop branch (each finished ticket). This provides a testable version of the application for all interested parties without requiring any manual work of building the app by developers. It also eliminates the unnecessary communication about which tickets are in which build and enables tickets to be tested as soon as they are implemented, without waiting for the build to be created.

WRAP UP

The disadvantages of this approach:

  • When there is a long-living merge request (because of its size, lack of time for review, or any other reason) it’s probable that a develop branch is going to defer a lot from a feature branch which will cause a big amount of conflicts. When resolving conflicts it’s easy to overlook something and cause a bug in the code-base later on.
  • If the team doesn’t empower all team members to actively participate in the code review process, only a few senior developers (by general experience or by the amount of time working on that specific project) might commit themselves to do the code review, while others remain to be just passive viewers. It’s important to develop a learning atmosphere which would encourage even less experienced developers to feel it’s OK to leave comments on other colleagues’ code.

The advantages of this approach:

  • The team provides better code quality which will produce fewer bugs, leading to less time spent on developing code overall
  • All team members will be familiar with all the parts of the system which will further enable the dev team to develop faster and avoid situation bottlenecks when someone is ill, busy, or away, on holiday.
interesting read? share it!

About the author

Sonja Brzak Software Developer

Sonja is a software developer with a special interest in Android and the world of mobile in general. She thinks good processes make room for creativity and is always eager to improve team collaboration.

Sonja Brzak Related posts

Latest blog posts
2021: The Year of Growth
Code Crafting Week - We Have a Winner!
Soft Skills Academy: Leadership and Productivity - Our Thoughts
GreenIT 0.2: United for Greener Kindergartens
Code Review — Why and How