Disclaimer: In this article I list the key points I consider every time I do a code review. This is not a step by step guide on how to do code reviews. I don't expect everyone to agree with the statements below. Finally, English is not my first language, so my apologies for any mistakes.
Code reviews is the time of judgment (this meant to be a joke) - it's the place where you give the opportunity to other developers to validate the piece of code you proudly worked on. Regardless you are a junior or senior developer, there will always be something you might have missed in your code, or could be written in a better way.
There are times where code reviews turn to be a battlefield between the reviewer and the code author though. But this should not be the case. Remember, the result of a code review is to merge a valid and clean piece of code into the code base. Thus, as reviewer, the comments you submit should be relevant, the wording proper and the observations straight to the point. On the other side, as the author of the code, any comments should be welcome and same thing here, any replies to be in proper language.
But anyway it's not always about code, there are few other aspects to pay attention when reviewing code. I list below some of my technical and non-technical criteria I have in mind every time I do a code review.
Technical considerations
Firstly I usually go quickly through the code in order to check for any obvious:
typo/ misspelling /readability, code coverage, linting, code smell, duplications in the newly introduced code, naming, magic strings and numbers. For most of these we can also rely on automated checks - Sonarqube for example is a widely used tool that helps to identify the mentioned issues upfront, reducing the manual reviewing time.
I then have a second look to check as for coding principles (example SOLID standards), simplicity and reusability. For big (usually legacy) projects, I try to check if any of the introduced code exists somewhere else in the code base so we can use the existing one to avoid reinventing the wheel.
Consider a better syntax. There will always be a better way to write a certain piece of code. But it is important to use the most up to date syntax and features of the language or framework we are using so we can take the most out of it, taking advantage of their functionality. Here I try to keep the balance between "you could change it" vs "you should change it" and this is because I try to respect the developer's programming style as well. In these situations I usually put as a suggestion the requested change, highlighting that this is how I would write that.
Non-technical considerations
What the ticket is all about. In an agile environment, every submitted piece of code should follow a previously discussed ticket (whether this is a story, a task or a bug). As a first step so, I check if the provided code, satisfies the acceptance criteria of the ticket. As part of this I try to identify any edge cases too - any use cases that might have missed from the ticket's criteria and they should be handled as part of this work.
What exactly the merge request (pull request if you like) contains. Some times we tend to sneak small amends as part of another merge request, in order not to raise a separate ticket and merge request. It is practical every merge request to contain (relatively) small changes and this is because if for some reason we have to roll back the master branch, then all this work will go wasted. Small merge requests are digestive - easy to review and be tested - so we can keep rolling smoothly.
Commit message is part of my reviewing process too. Looking the commit logs in a later point of time we should have a list of meaningful commits and even better, to contain the ticket number where this change had to be introduced for. To get this further, having straight to the point commits can be useful for when investigating (possibly as part of debugging) an area into the code - a git blame on a specific line of code will reveal the commit message so it makes sense to meet a meaningful commit in order to understand the existence of this line.
Finally, I want my reviews not to affect productivity so before I leave a comment I have always in mind a couple of things which I group together here:
- Time spent on the ticket. If a ticket is under development for too long, then adding less important comments it will delay the delivery even more. In these situations I try to leave the necessary comments - any lower priority observations can be addressed in a later ticket.
- Urgency. If a ticket needs to move fast (for example bug on production) then I skip the steps for better syntax, naming etc. For an urgent bug, I am more open to allow a quick solution from a more proper one, in order to resolve the direct problem and satisfy the end user, but always follow up to improve this piece right after. Remember, our projects have to be as perfect possible in terms of code but they should also be functional for the end user.
Code reviews is a chance to see the problem from different angles and consider different (or maybe better) ways to resolve it. Both sides should be open to submit and address valid comments on the code. There should be no ego or arrogance in this - no hard feelings at all! Better few and meaningful words (pointing some documentation and resources if needed) from a tone of comments that may not make sense in some cases.
Thanks for reading this. If you want to show me some love, you can always ☕ Buy Me A Coffee
Top comments (0)