In my time as a developer, I have given hundreds of code reviews. It's something I learned to enjoy very much as it gives me others' perspectives on our codebase. Most days I learn something new from it. In this article, I want to go over few tips on how to be good at them and especially how to enjoy them.
Good pipeline
Most teams have some kind of code-style rules and recommendations on how to uniform their codebase. Whatever you do, do not force code reviewers to check whenever the pull request complies with those rules. There are tools to do that look at eslint, SonarQube. These tools should be run as part of your PR pipeline.
SonarQube is one of those tools that will improve code reviews for everyone in your team. It can find style issues but can do so much more. With their code analysis, they can find the most common mistakes you can do in your code. You never closed your resource? Sonar will let you know. Personally, I never do code review before Sonar finished its job.
You need to have a concrete process that will run every time somebody creates a PR or pushes new changes in there. Steps can look something like this.
- Compile the code
- Run the tests
- Run linting
- Run SonarQube
- Do the manual review process
- Merge to the main codebase
Flyover
Like first thing, in my review, I will do a quick overview of all the changed files. This I usually do inside the pull request UI. I am focusing on few things during this stage.
First is the code readability. How is the reading experience? Is it apparent what is being done? Good code should be able to convey its purpose at first glance. Then I am trying to figure out if this change is what is required by the corresponding change request. I will read through the ticket and try to match all requirements there in the pull request.
During this stage, I do not dive deep into the implementation details. I identify those for later stages.
Possible points conveyed at this stage:
- change is not very readable
- change does not cover the requirements, or change request ticket was not changed as requirements changed
- missing tests
Go deep
After the brief code flyover, I have a list of changed files I need to go deep into. I usually do this in IDE so that I can see connections between files better, but for most changes, it's ok to do it in the web UI of the pull request.
I start by asking the question of how would I solve the problem and go from there. Is there a piece of code in our codebase that does something similar (or the same)? Is there a library (that we currently use) that can be used for this? If there is a library that we do not use currently is it worth adding to solve this issue?
Test review
Do not forget to review the tests. Are all cases valid? Does those tests have the possibility to find something? Are assertions readable? Are tests even present?
Do not be afraid to fight back to add more tests if you as a reviewer feel like there is a possibility to add them to improve the codebase.
Be nice and educate
Whatever you do, do not be arrogant, snarky, and sound like a know-it-all. If the proposed solution is valid, but you would do it differently you may say so but also approve the request. You may leave education comments, but too much can of those can be harmful. Consider what feels important. Do not argue about little things, people are different and have different opinions.
Approval
Be quick on approval, not perfect is ok.
This is the golden rule you should go by.
For more tips like this, you can follow me on Twitter.
Top comments (10)
Great article! I've written about code reviews too (hope it adds value to your readers): dev.to/codacy/top-10-ways-to-perfo...
Sonarqube is a great tool indeed. Would like to know your feelings about Codacy :p
My process is just about the same!
I feel like the flyover is important part of the process. That way if something isn't working correctly, I'll ask them to please fix the issue before I continue to complete the review.That way I don't have to repeat the more difficult second step twice (which validates true correctness).
Articulation of educational suggestions is extremely hard and can be perceived differently by many. One learns this only by experience working with different types of people. The code review process itself requires a lot of skills. Being a good developer is not the same as being a good code reader. People should practice reading code to be able to spot what might be bad and what might be a different way of doing something than their own approach.
A good thought-provoking topic nevertheless with some good suggestions. Kudos!
Agreed! 💯
thanks for sharing! good advice to get better at code reviews!
Great workflow @pavel_polivka 👏
This workflow can be really helpful if followed.
Just so you know, there's a reddit user reposting your article w/no attribution across multiple subreddits.
reddit.com/r/learningCode/comments...
Thank you for noticing.
I wrote him a message and he deleted the whole profile.
So helpful, thank you.
Really nice workflow. I've also suggest some other tips at dev.to/marcelosousa/six-best-pract...