Importance of Code reviews
Inspired by some of the last few projects, I have noticed that still a lot of people don't consider Code Review seriously.
From extreme cases where code review doesn't exist at all or where pull/merge requests are approved in a few seconds after being opened. Over the cases where the minority of team members care and perform code reviews, and the rest of the team members ignore them. The funny thing is when they are explicitly asked to check it, they consider them as "punishment".
Luckily, one of my first projects had a really good code review process where all team members were very active and performed strict but fair code reviews. On that project, I learned a lot, learned why this process is so important and gained really good habits on how to perform code reviews.
That's probably one of the reasons why I'm writing this today.
Recently I was preparing guidelines and best practices about code reviews so I decided to share them with you as well.
The things you will see here are not only my statements, they are inspired by other people, teams, and companies. But with those statements, I totally agree and I stand behind them.
So what is code review?
A code review is a process where someone other than the author(s) of a piece of code examines that code. Code review should be used to maintain the quality of our code and products.
Process Flow
Once you're satisfied and proud with your implementation and the tests, you create a pull request. With a pull request, you are requesting the merge of your local branch into the main source branch.
All dev team members (including Leads / Architects) should be default reviewers of the pull request. The reviewer's job is to check if all requested functionalities are properly implemented and tested. She/He can suggest code improvements like using some library instead of reinventing the wheel, better naming for components and variables, and any other improvement that she/he can suggest.
It is important that you shouldn't be disappointed if you receive a lot of comments on your pull requests. You will learn from the feedback and the solution will be better.
When the minimum required number of reviewers have approved the pull request you can merge it. Don’t forget to delete your source (feature/bugfix) branch and move the ticket that is ready for QA (after it's deployed).
Author of the pull request
Goal: Learn, improve your code quality, morale, and your working relationships.
REVIEW your own code first
Before sending code to your teammate, read it yourself and check the things that you would check as a code reviewer.
Be UP TO DATE with the develop branch state
The recommendation is always to be up to date with the latest state from develop branch. This means that during the day you should pull changes several times and merge it to your local feature branch.
Before creating a pull request make sure that your feature/bugfix branch is up to date.
Pull request should be DESCRIPTIVE
- Branch Name
- Source branch name should consist of branch type (feature, bugfix, hotfix), Jira ticket number, and Jira ticket title
- e.g feature/ticket123-some-ticket-title
- Source branch name should consist of branch type (feature, bugfix, hotfix), Jira ticket number, and Jira ticket title
- Commit messages
- Commit early, commit often with a descriptive message what you did
- Pull request description
- Add an additional summary of what is being done if it’s not clear from the pull request title
- Point out not so obvious/logical decisions and agreements
- Comment your decisions
- Comment yourself (inline in code) for not so obvious/logical decisions, so that reviewers can easily understand it
Prefer SMALLER pull requests
Try to make your pull request small as possible.
Smaller pull requests:
- easier to review and understand
- faster to review
- less chance to miss something
- less chance to create a bug
Respond graciously to feedback and LEARN from it
Don’t take personally a code review feedback. Treat your reviewer’s notes as an objective discussion about the code.
Be PATIENT when your reviewer is wrong
From time to time, reviewers are wrong. Just as you can accidentally write buggy code, your reviewer can misunderstand the correct code.
COMMUNICATE your responses explicitly
For every comment that requires action, respond explicitly to confirm that you’ve addressed it. Some code review tools allow you to mark comments as resolved. Otherwise, follow a simple convention, like, "Done", for each note. If you disagree with the note, politely explain why you declined to take action.
Don’t directly ask/chat with the reviewer about every comment. Commenting and discussing directly in the pull request is useful for other reviewers as well.
MINIMIZE lag between rounds of review
Once you create a pull request, driving the review to completion should be your highest priority. Delays on your end, will waste time for your reviewer, and increase complexity for your whole team.
RESOLVE Conflicts
In case of:
- disagreements
- totally wrongly implemented features
- too many mistakes
- doubts, unclear things, or comments
schedule 1 on 1 call with the reviewer to resolve all doubts and unclear things. Make sure that you comment back agreements directly in a pull request for other reviewers.
Code reviewers
Goal: Mentor, educate and improve your working relationships.
What do Code Reviewers look for
- Design
- Is the code well-designed and appropriate for your system?
- Functionality
- Does the code behave as the author likely intended?
- Is the way the code behaves good for its users (end-users and developers)?
- Complexity
- Could the code be made simpler?
- Would another developer be able to easily understand and use this code when they come across it in the future?
- Tests
- Does the code have correct and well-designed tests?
- Do the tests cover requirements?
- Naming
- Did the developer choose clear names for variables, classes, methods?
- Comments
- Are the comments clear and useful?
- Style
- Does the code follow the company style guide?
- Documentation
- Did the developer update relevant documentation?
Don’t criticize, MENTOR and EDUCATE
Code review is a chance for developers to mentor and teach others.
When you comment on someone's pull request:
- be kind
- instead of criticizing, ask why decisions were made like that
- suggest improvements
- explain your reasoning
- balance giving explicit directions with just pointing out problems and letting the developer decide
PRAISE good code
Remember to praise good code and decisions. Don't only point out problems and suggestions.
Don’t ignore pull requests
Don’t ignore pull requests and think someone instead of you will do the review.
Make a habit of doing code reviews like in the morning before daily, around lunchtime, before the end of the working day.
If you are in the middle of a focused task, such as writing code, don’t interrupt yourself to do a code review. Instead, wait for a breakpoint in your work before you respond to a request for review.
Less experienced team members don’t be shy
Your opinion is important and expected here as well. More experienced team members don't know everything and they also make mistakes.
Speed of Code Reviews
The speed of individual development is important, but not as important as the speed of the entire team.
The speed of the team as a whole will be decreased if the individuals don’t respond to the pull requests, do the review, and push other work (task) in the done direction.
If you are not in the middle of a focused task, you should do a code review shortly after it comes in.
Code Review Improvements Over Time
If you follow these guidelines and you are strict with your code reviews, you should find that the entire code review process tends to go faster and faster over time.
Developers will learn what is required for healthy code, and when you create a pull request that is great from the start, it will require less and less review time.
Reviewers will learn to respond quickly and not add unnecessary latency into the review process.
But don’t compromise on the code review standards or quality for an imagined improvement in velocity—it’s not actually going to make anything happen more quickly, in the long run.
References:
