Having a reliable wingman at your side can get you places. And I don’t mean another pub or nightclub.
Developers who consciously decide to check each other’s code will generally have a much simpler life launching and supporting a scalable product. That’s where code review best practices come into play.
Main principles of a code review
- Make sure you have access to all necessary information: Understanding the context of the code you are reviewing is critical to the success of the process. Few things are worse than getting stuck in the middle of a review because you lack some insights on how and why the software is designed to work. You can use a debugger to learn more about the code before you dive deeply into the review.
- Good feedback is key: the feedback you provide should be specific and actionable. This can be quite challenging for some teams as people will subconsciously dislike someone else pointing out their mistakes. It’s just human nature. So, instead of showing what you believe to be an error, try explaining your reasoning. One of the tips we at Railsware use ourselves is a policy to ask questions instead of making statements.
- Have a clear definition of done: When is the code considered reviewed? What are the criteria for accepting it? How is it tested and which tools are used? Answers to these and similar questions come from having an established, reliable, and repetitive review process. One of the best approaches to succeeding in this task is a code review checklist.
Code review checklist
Deciding on how deeply you want to go with a checklist depends on your team, the scale of the project, availability of resources, and overall business culture in the company, and your Jira workflows. That said, there are some basic elements everyone needs to include in their code review checklist:
- Verification of feature requirements: A developer’s goal is to make a product that’s convenient and usable. That’s why, when reviewing a feature, we must have a clear concept of whether this functionality is a) useful to the user’s goal and b) coded in the most efficient way.
- Code readability: Nobody wants to dive into a bowl of spaghetti unless they are on a lunch break in a trattoria.
- Coding style: best practices and patterns exist for a reason – they’ve proven themselves to be among the most efficient solutions for generic tasks (those that do not require custom business logic). Sticking to them is a must.
- Clear naming: People prefer order. Software is dependent on it. Why make things harder for both?
- Code duplication: Is there any duplicated code?
- Tests: Covering 100% of the code in unit tests is an absolute business necessity. There’s no getting around this one.
- Documentation: Every time I write about documentation the ‘we live in a society’ meme pops in my head. The thing is – we do live in a society and making life easier for others adds to the longevity of our products, the ease with which we can onboard new teammates, and to our karma.
Yes, at first glance the list seems a bit basic. But going deeper into each of these points will offer the needed clarity for effective communication and successful completion of the task. Let’s take these points and make an example checklist based on such core principles as verification of requirements, readability, formatting, and tests.
Step 1: Add the Smart Checklist add-on to your Jira instance. You can try out the free version, but I would suggest upgrading to pro in order to have access to more than 20 items per checklist.
Step 2. Choose your site and follow the instructions. Do note that you must be an administrator. Otherwise, you can request the installation of the Smart Checklist add-on by clicking the ‘Request app’ button.
Step 3: Create a new issue and you’ll see the checklist option there. You can add new items to the checklist via the field below, or you can open the Markup editor.
Step 4: Create your checklist. I’ve created an example checklist you can simply copy and paste into your Markup editor. Feel free to use this guide in order to make changes and edits.
# Code formatting check
- Are alignments, proper white space, and easily identifiable code block starting and ending points present?
- Has the developer followed correct naming conventions?
- Can the code fit on a standard laptop screen? You shouldn't need to scroll diagonally on a 14’’ screen.
## Architecture check
- Is it split between presentation, business, and data layers?
- Is it split into files based on technology?
- Is it working with existing technologies and patterns?
## Coding best practices check
- Is the code using constants?
- Are similar values grouped under an enumeration?
- Are the comments informative and beneficial? Do they explain what is being done?
- Is there an excess of if/else blocks?
- Is the code relying on framework features wherever possible rather than relying on custom solutions?
## Maintainability check
- Is the code easily readable by a human being?
- Is the code easily tested?
- Can it be refactored into a separate function?
- Are there flow of control, parameter, and data exception details necessary for debugging?
- Are configurable files kept in place? There must not be a need to make changes if the data changes frequently.
## Reusability check
- Are the same principles not repeated more than once?
- Are reusable services and functions used when applicable?
## Security check
- Validation against SQL injections
- Validation against XSS
- Is sensitive data encrypted?
## Scalability check
- Is the code scalable?
## Useability check
- Is the solution usable from the user’s POV?
## Performance check
- Is the correct data type used?
- Lazy loading, asynchronous and parallel processing
- Caching and session/application data
The end result will be a checklist that looks like the one below.
Why use a custom checklist?
Jira does not have its own checklist functionality, but there’s nothing a couple of plugins (one, in our instance) can’t fix. You can use Railsware’s Smart Checklist for your Jira instance.
This plugin allows you to create custom ToDo checklists, add notes and statuses to them, assign specific people to certain items in the list, and more.
Having a checklist ensures that the developer – even if they are new to the team – will have a clear understanding of the process as well as a point of reference in case some steps have nuance or require specific knowledge.
Some of the extra features of the Smart Checklist include:
- Custom statuses
- Permission management
- Editing in Full Screen mode
- Templates for Definition of done and acceptance criteria
- Markdown
Code review: Pro tips & own practices
I’ve decided to break this section down into two segments. The first section will mention some of the better known practices and the other will offer insights from our own policies and workflows at Railsware.
1 issue – 1 pull request
This way, we can reason about which changes were made on a particular branch. This way, you understand that every line of code is made to benefit the feature rather than miscellaneous refactoring or any other means.
Git has some built-in features to show which branches have been merged. So if you have a mapping between features and branches and branches and pull requests, then you can use the git branch merged command to see what you are about to ship to your customers. Or use not-merged to find out which features are not merged to your master branch. Jira and bitbucket have built a tight mapping between Jira issues and branches so developers can use triggers to transition issues based on events happening inside the repository.
All the developer has to do is assign the issue to themself and then create a branch and push it to the server. Then, Jira automatically tracks the state of your development.
Minimum 2 approvals before merge
An interesting correlation has been found between the number of reviewers you assign and the number of total issues found. Surprisingly enough, the more people you assign, the fewer errors they’ll find.
This may be because the team has too much confidence in someone else reviewing the code.
As a general rule of thumb, I’d suggest having at least two people other than the author signing the changes. This way, you are not over assigning, and you’ll always have someone who knows the code so the Buss Factor is not a threat anymore.
Managing issues stuck in review
Developers prefer to write new code rather than review someone else’s code. This leads to issues being stuck in review for an extended period.
Pick a time during the day to make code reviews. Let developers go through the reviews and merge the changes that have been approved and/or provide feedback on outstanding pull requests.
Request reviews on your own
For starters, let’s keep in mind that a review request is not the same as a pull request. It should be a separate step, because there are no reviews by default when one submits a PR. You, as an author, will need to add reviewers manually from a corresponding tab. GitHub will also suggest reviewers based on their commits.
The number of these reviews will vary depending on the scale and complexity of your project. We’ve found that 2 is good for smaller teams and 3 is usually plenty for larger projects.
Again, this step is great for a checklist as you need to get the context across to the reviewer.
This process is already well-described on the Railsware blog. Feel free to check it out if you are looking for more detailed steps or tips on using pull requests in your code review process.
