Why do we do Code Reviews?
Code review is simply what it says on the tin. It is the opportunity for other developers to look at your code commit and review it.
Code reviews are one of the key elements to the development lifecycle. Without them poor or buggy code could make it's way into UAT and slow down the testing / release process.
My tips
Word comments carefully
Remember that the person's code you're reviewing has probably put a lot of effort into this work, and they haven't deliberately made mistakes, especially those easy to miss spelling mistakes etc.
Try not to be too soul crushing when leaving comments.
Don't be so forceful with comments or suggestions. For example rather than saying
Change this variable name
Say something like:
Perhaps this could be worded to a more descriptive variable name.
This way the suggestion for improvement is made, but in a way that makes the coder feel they still have a say / matter is open for discussion.
Be clear and concise.
Be straight to the point and clear about what you want changed or are advising. Don't waffle on, beating around the bush simply state the What and Why.
Most git systems these days allow you to click on the line you wish to be changed , and add a comment so it's much simpler to specify the exact line of code you want changed.
Hosting providers such as GitHub have a "suggestion" feature which allows you to add a code suggestion directly into the comment, which can instantly be accepted and committed from within the PR.
Multiple changes of the same kind.
Rather than commenting every single occurrence of the problem (this can be overwhelming for coders. Simply add one comment and explain the issue occurs throughout multiple files, and suggest finding all occurrences and updating, e.g a spelling mistake, or repeated code.
Not everyone codes like you
Remember that not everyone codes the same and certainly doesn't always code the way you do. This doesn't mean they are wrong, and nor does it mean you're way is best.
Always take a step back and think about the key elements of code review.
Does the code follow your team's coding guidelines
Does the code meet its objective / acceptance criteria.
Is the code legible and easy to understand what it's doing without the need for heavy comments / documentation. (This one for me is one of the most important, as I'm a huge fan of descriptive method and variable names.
Does the code need any refactoring, considering security, performance or simply ease of reading.
Does the code follow simple design patterns / principles e.g single responsibility, abstraction, encapsulation etc. If not make suggestions on how this can be achieved or perhaps teach those not familiar with it what it means and the benefits.
These are some of my tips I hope they've been helpful.
Feel free to discuss your tips and methods of code review in the comments.
Top comments (10)
Good points! I prefer giving concrete examples about names rather than use terms like "more descriptive". What's descriptive is subjective and I've been in situations where the PR author doesn't know what to change it to.
Tests is one thing I keep an eye out for. If there are none I don't approve it unless for good reasons 🙂
This is great advice and a reminder of the pull request reviewer's role: gatekeeper. Without their approval the code doesn't get merged. With their approval it can get merged. And everyone reviewing code should understand this positional power.
The review is the time of inviting others to help shoulder the burden of maintaining the "main" branch. It is a distinctly collaborative moment that both shapes and reflects the state of the code base. Review with a robust concern for the code quality and for the person offering the code.
Adding to the list:
is objectively more clear and concise than
But I too usually fall into the second while trying to be polite 😂
Except this time I declined a PR that used random generated numbers to IDENTIFY things 😅 The guy come and said that it worked in hims machine and that mathematics are exact science. I've lost 30min explaining why this is true unless you add randomness into the algorithm.
Sorry for the long comment, that's to say that CRs are fine but most of the time we need to group together with the juniors (and not so juniors) to make them truly understand what was wrong as mentoring process.
I always try to enforce KISS as base principle, then comes SOLID, guidelines and so.
Totally agree. The other day I got a comment on my MR about a trailing space.
Java developer, My reviewer usually points outs me over complicating things like:
Is that what they mean by KISS 😅?
if you got the comments such as "using a class for representing Money" several times, it might be a good idea to talk about it in another dev-jourfix or retro meeting. The goal is to clarify if your team can do a design decision together, if we apply an ValueObject Money instead of using BigDecimal generally. If yes, just put such thing into your team coding convention. Then everything will be easier in the future.
This is really sound advice everybody has a different way of doing a code review.
Great article!
Excellent points! Rather of using phrases like "more descriptive," I like to give real examples of names. What is descriptive is subjective, and I've been in instances when the PR writer is stumped. iogames-free.com
Very good points! On my team we take in consideration all this points! 🥰