In the 3 and a half years I've been working as a software developer I've done a lot of code reviews, below are the questions I ask myself and my general thought process when reviewing code. I’d be interested to hear what other people do the same or differently to me - please do leave a comment once you’ve finished reading!
Before looking at the code, I read the ticket description
Where I work we are assigned relatively small tasks called tickets, so the first thing I do is read through the ticket before looking at the code review. This may seem obvious (or unnecessary), but I think it's important to know what the code is meant to achieve as well as checking that it makes logical sense and works. I like to have this in the back of my mind as I go through the code review, so I usually keep the ticket open on a different screen so I can look at it alongside the review if needed.
Then for each code change I encounter, I ask…
1. What is this change doing and Why?
The what may be as simple as removing import statements, adding an if check or creating a new method.
The why could then be "I remove these imports as they're no longer used", "We need to check for this condition before doing the below" or "I need to add in this new functionality"
For changes like adding a new method I would ask this for the bigger picture - what is this method doing - but I would also ask this for each line/code block within the method.
For tests I would ask: what case is being tested, what is the expected outcome and why?
If it takes me a long time to understand this part, it could be because I am unfamiliar with the code base or practices used, but it's worth adding a comment to ask if you're not sure what's happening. If it’s quite complicated then perhaps the code could be slightly refactored, a method/variable could be renamed or a comment added in the code to make it more clear.
Once I understand the logic, I ask myself Should this be happening? Does it align with what is specified in the ticket description?
What is not happening here? Can I think of any other cases that should be accounted for? Are there tests?
Once I understand the what and why, I then think about How (for changes more complicated than just removing unused code).
2. How is this code change achieving what is intended?
Will this functionally do what is required by the ticket?
Can I spot any potential issues where this may behave incorrectly?
Are there any improvements that can be made? This could be performance-related or otherwise, e.g. could an if check be short circuited, is there repeated code that could be made into a method, are any parts not used and can be removed?
Can I think of any other cases that could be caught here? This overlaps a bit with what I said in the previous section, but this is slightly different because we’re looking at a lower level here - will the code behave correctly in all cases, e.g. is a null/empty/invalid input handled correctly?
3. Then I think about Where - where is this functionality used?
This is particularly for changes that would affect other places, like changing a public method’s signature or functionality, or a stored procedure.
Where is this functionality used and are there other places that already use this functionality and might need to be edited to accommodate the change?
Have all instances been updated that need to be? E.g. for a ticket to update all instances of X check, have all the X checks been updated or might there be more somewhere else?
If we’re using a variable from elsewhere, what is its value and does it make sense here? E.g. if a string variable is used, check the original string and make sure the wording still makes sense for this use case.
Sometimes I will search for this myself in the code base if I have an idea where to look, otherwise we can leave a comment to ask if the author has checked this.
Source: Saving the world with a code review
4. For text or messages that are displayed/returned I would ask - Who will see this message?
If an external user/customer will see this message, would it make sense to them or does it just make sense to the developers who wrote it? We may well need input from someone in a different department (like Business Analysis or Product) to help with the wording. Another consideration is do we want to display this level of information to external users, or are we telling them too much about our backend systems?
If this is for messages being logged, will this message make sense if you come across it in a log file? Could it do with more context, or a reference/id so that we can debug more easily?
If there are existing code comments, do they need updating? For new comments, do they accurately describe what’s happening - especially given things may have changed even during the review process.
5. For automation that is triggered or on a schedule - When will this be run?
This is something I come across more rarely, but for this sort of change I would check this generally makes sense and that it aligns with what is outlined in the ticket.
6. Finally, after reading all the code, am I satisfied that this will achieve all the ticket’s goals?
Usually the answer is yes, but if something seems to behave differently or is missing then I would leave a comment to ask about it. Of course, things may have changed since the ticket description was written, but even so it’s worth asking to make that clear to you and to other reviewers.
Extra Idea: leave a positive comment!
So often the only comments I leave on code reviews are negative, and I just wouldn’t leave any comments if I don’t find problems with the code. After watching the video mentioned below, I’ve been challenged to write a positive comment in code reviews to help encourage my colleagues. Not to force it or be patronising, but if there’s something I genuinely think is good or interesting then instead of keeping it in my head, why not put it in a comment as well.
Thank you for reading until the end, I hope you found some of that interesting! Is there anything else that you think about when reviewing code? What do you do differently?
Recommended watching/reading
“The Art of Giving and Receiving Code Reviews (Gracefully)” by Alexandra Hill (under 10 minutes) - https://www.youtube.com/watch?v=XY6eA2_2hOg
I found this very useful, in particular the “As a reviewer we can…” and “As an author we can…” sections, helping us to bring out the best in each other when reviewing code.
(Here is the equivalent blog post if you’d rather read it: https://www.alexandra-hill.com/2018/06/25/the-art-of-giving-and-receiving-code-reviews/)
Source for the title picture: Code Review
Top comments (0)