Part two of code reviews.
In November, we described Code Reviews as being an essential component to our L4 Digital delivery process. We hope that our article has given some of you ideas for your team. After looking at the landscape of products that exist in the market, it might be difficult to know where to start. Do you want to do pull requests of a branch or review on a commit by commit basis? Do you have budget concerns, hosting requirements, or a repository that already has reviews built in? How do you do a code review once everything is set up? We’d like to help by walking you through our implementation and some of the decisions that you will encounter along the way to getting Code Reviews into your workflow.
Step 1: Gather requirements
Integrating something as big as the code review process and a system to support it should be done in a thoughtful way. A few years ago when we were setting up our code review system at L4 Digital, we had a list of requirements that our new system would need to fulfill:
- It needs to be built with the benefits of Code Reviews at the core. This means that it should allow one to find and fix issues as early as possible, facilitate team collaboration through tools, and enable the review of the actual code in a manner that is as clean as possible.
- It should be a product that is supported, maintained, has extensibility, and such a presence to where it might show up in an internet search or user support groups.
- It must support and integrate with our current tools, Git, Jenkins, JIRA, and Slack.
- The code review component cannot be an optional part of the system. Everything that reaches our QA team will have to have been reviewed along with a log of how the code made it to the build.
Your team might also want to think about:
- Your existing repository. Is the system you are using for version control already set up for code reviews? Products like Github, Bitbucket, or Stash have this built in, but is their implementation right for your workflow?
- Hosting requirements. Do you have the ability or need to host the system yourselves or do you want a hosted solution? Can your IT department support them?
- Budget. Do you need a free solution? A one time fee or per seat license?
- Maintenance. Do you have an IT department that can support the solution you choose? Can they keep up with upgrades? Who will be in charge of the users, permissions, and settings?
- Security. What kinds of security requirements do you have? Do you need to have locked down permissions for different types of people or groups?>
Step 2: Identify a suitable solution
After the requirements have been gathered, use them to identify a solution that best matches the needs of your team. At L4 Digital, our two finalists were Gerrit, a review system originating from the Android Open Source Project and Github, a web based Git repository hosting service that also includes review components to their solution.
|Huge community support, many open source projects are hosted with GitHub.||“Bad code” could end up in the main branches. In a typical pull request with feedback, a person would add commits to address issues. Once the branch is merged all of the commits are part of the history of the master branch including the ones with errors.|
|Clean user interface with many options. One can use a web based, application, or command line to fit their preference.||To avoid the bad code, one would rebase and force push code to the PR. When this is done it’s difficult to see how one might have addressed the feedback of the previous code and difficult to see the changes from the original request and the current one.|
|Uses Pull Requests (PRs) for code reviews. Can be easier to understand for someone who hasn’t worked with code reviews before.||Pull requests can lead to a cluttered revision history making it difficult to understand context of individual commits.|
|Can be hosted either at GitHub’s servers or locally in an enterprise installation.||Missing some important features of Code Reviews like a review rating or score, validation from a build server, and permissions on those features.|
|Not open source.|
|Single fast forward commits are much better than merge commits to the target branch. The history is uncluttered with all commits individually reviewed.||The User Interface can be overwhelming at first feeling more difficult to use or cluttered for new users.|
|Permissions are very customizable with the defaults being good for most people. It allows one to customize access to particular projects as well as branches but also each type of action that one could do to the repository.||It must be hosted and maintained by your own team.|
|The review workflow avoids bad commits getting into the important branches. Feedback can be seen and addressed so that only the final “clean” commit is in the repository; but also allows one to see the issues and feedback in the review history including differences between patches.||Requires users to understand more of the Git features including rebasing and cherry-picking.|
|Open source with plugins for things like Authentication, Automation integration, Mirroring, and Defect Tracker system.|
Step 3: Installation
We found that the Gerrit code review system worked well in meeting our requirements for the process. Gerrit can be found at: www.gerritcodereview.com. It’s Java-based so you can pretty much install it on any server. Tutorials for installation on your favorite flavor are easily found in an internet search. In addition to the default Gerrit installation, we use the following tools in the system:
- Google Auth Plugin – Gerrit allows customization of authentication providers, and davido on Github created a plugin for Google authentication in Gerrit here: github.com/davido/gerrit-oauth-provider.
- Gerrit-Trigger Jenkins plugin – Some of the review process can be automated. If code breaks a build we don’t need a person to look at it. If a unit test fails then it fails code review. We have a Jenkins build for each project that will verify our code review requests before another peer will take a look. This build will first verify the code builds properly then will run through the automated tests for the project. If they fail then the code review will not pass and others shouldn’t need to look at this code until a new patch is uploaded. This is done using the Gerrit-Trigger Jenkins plugin found at: https://wiki.jenkins-ci.org/display/JENKINS/Gerrit+Trigger.
- JIRA plugin – JIRA has a plugin for Gerrit that will show the status of a Task in the system. The key to linking the JIRA task and the Gerrit changeset is to make sure the commit message references the JIRA task number. For example “JIRA-1234 updating the app icons” in your commit message will then link the Gerrit item to your JIRA item. The plugin can be found here: https://marketplace.atlassian.com/plugins/com.meetme.plugins.jira.gerrit-plugin.
Step 4: Roll out to the team
Finally, your team should have a clear definition of what should be done in a Code Review. Create a checklist to reference with the team and follow through with the items in the review. Put this on an internal document or wiki so that the whole team understands what is needed to do a Code Review the right way. Walk through the document with the team and new hires as you grow. The following is a snippet from L4 Digital’s Code Review checklist:
- Verify that the code addresses the JIRA item’s acceptance criteria. Does it handle both the happy path and error case scenarios? The review should be addressing a Jira Task or set of tasks. If there are parts to this commit that don’t fit, then it should be part of a different review.
- Look at the changes and verify the team’s coding standards are met. For example, does it follow consistent naming conventions?
- Does the code follow the platform architecture pattern (like MVC) properly?
- Is there commented out code? If so, is there a good explanation why? If not it should be removed.
- Are there things like //TODO in the code? If so, it should have a JIRA task listed so that this won’t be forgotten in the future.
- Unit test failure, static analysis, build warnings and errors shouldn’t exist and should be caught by the build, but if not it should be fixed before approval.
- Methods should be a screen size (or two) in height or less. It is much easier to understand code when you can read the whole function on screen.
- Class files shouldn’t be too large. When they get over 500 lines they should be examined to see if it makes sense to split things out into logical pieces.
- Text strings should not be in code, they should be sent through the project’s localization functions.
- Code reviews and critical feedback are not a personal judgement. A majority of code review requests that are posted require additional work to complete a feature. This is a surprise to many people. This isn’t a rejection, but rather a support system for getting features implemented as close to perfect as we can the first time.
Links to other useful resources:>
Gerrit Best Practices: http://wincent.github.io/gerrit-best-practices-tech-talk/assets/fallback/index.html
Bad “smells” in code: https://sourcemaking.com/refactoring/smells
More Code Review Best Practices: http://kevinlondon.com/2015/05/05/code-review-best-practices.html
Step 5: Go!
We hope you’ve enjoyed the read. If you choose to integrate code reviews in your next project, our points for bringing tooling and practices into your own shop are a great place to start. To some, code reviews are a special occasion, but at L4 we’ve woven them into our daily process to great effect.
Thanks for reading, and let us know how it goes. If you need more information on why we use code reviews, visit Code Reviews: The 13th Step To Better Code, Part 1.
Image courtesy of Luis Toda for Unsplash.