A legend once said "Quality of code is measured in the number of what the hecks per hour". We do not know who that legend is but in the meantime lets talk at Code Reviews.
Love it or hate it, you cannot run away from it. If you have been into any kind of application development, you would have done code reviews or had your code reviews. You would have provided review comments or had to address some of them yourself. Sometimes its a joy and sometimes its like.. you get the point.
I'll use this post to share how I perform code reviews and how it benefits me. I am certain this would be beneficial for you and secretly hoping that you will be more interested in performing reviews after reading this completely.
Why I love code reviews.
Here are the reasons why I invest time to review code. (Pro Tip: Do not forget to read point number 5)
While reviewing code I get a chance to understand how my team thinks, how they design, what approaches they take in order to solve a given problem.
I would get to learn something new. A new way of doing something, some interesting play with syntax or a library which I had not used previously.
Asking questions via review comments helps me get a deeper understanding of the problem or the solution implemented.
Since I'm investing time in code reviews, If I get to work on the same feature which I reviewed, I will be better prepared and would have a better understanding. I can complete the task quicker. Win Win !
I get a chance to praise a well crafted code. If you are not doing this, pls pls start from today. You will know its well crafted when you see one.
Code Review Etiquettes
Here are some points I always keep in mind while performing code reviews.
The task is code review. I ensure I only comment on the code and never on the Author.
Code belong to the entire team. Use We and avoid You or I when providing comments. Ex: "Can we try to use array.filter here" vs "You need to use array.filter here". While they both convey the same message, the tone is so much better in the former.
At times, even If I think I may know what exactly the code does, I may be still missing something. I always try and inquire about it rather than assuming things and commenting incorrectly about it.
Ex: I may be missing something here but does this work as expected when the object returns null ?
I read the user story/task to get a better understanding of feature being added or problem being solved.
Assuming the change is not small, I pull the code to my machine and run the feature and try out the positive flows.
By this time, I'll know what is working and what is not.
Now, with the help of tools like GitHub or GitLab or others (where the Pull/Merge request was raised) I skim through the code changes (differences) on a high level. These tools make it easier to identify the changes easily. In case I notice something odd, I add comments and categorize them accordingly. More on categorization in the following section.
My next focus would be to understand where the core logic of this feature/task resides. I'll spend a good time understanding this and wrapping my head around this. If necessary. I'll read the user story/task once again just to be sure.
I run the application again locally and try out different scenarios. This is where I get to understand things deeply and gain confidence of the change. Remember the code belong to the team.
Once the major logic is reviewed, I'll move onto other parts of the code and review them. While all these are being done, I'll make a note of my review comments either directly into the PR or into a notepad and eventually share them.
When necessary I make use of the Source Control Feature of Visual Studio code. I can see commit wise changes for the file being reviewed. This helps me understand how the solution evolved.
I try (its not easy :P) to review every line of code written.
After I finish the review, I make sure to add a general comment to the PR saying "I'm done" along with a positive compliment. I find this useful when reviewer and author have a common time zone or overlap.
Code review comments need to communicate the intension of the reviewer without a doubt. This is especially needed where teams are split geographically or working remote always.
I follow the Conventional Comment format to convey my intent about the code and follow the below format.
<label> [decorations]: <subject>
Lets dive in.
Labels
Here are the labels that I use frequently:
- nitpick
- suggestion
- thought
- issue
- question.
Lets look at examples of each of these labels.
nitpick: I use nitpick a comment for this which are not super important.
Example : Lets say a comment from one function was copied into a new function. Happens right?
suggestion: Usually for variable name suggestions or sometimes for doing the "same thing" but in a better / performant way.
thought: A different way of solving something. Its same as a suggestion but the key difference here is: I may be not completely aware of why something was implemented that way. And I supply enough description as to why I think we can try another route to solve the same problem.
issue: May be I tested the code and something is broken. Or I may have a better understanding of the feature and I for sure know "this line of change" will break something else either immediately or down the line. These comments should be resolved or discussed & aligned accordingly.
Decorations
- Blocking
- Non Blocking
- If Minor
- If Aligned
Blocking : Anytime I decorate a comment as blocking, its probably something we as a team need to change unless I've missed something. Blocking decorator means unless its addressed, the feature/task would not be working as desired.
Expected Outcome: Author needs to take action on blocking comments. Either change/fix the code or help me understand why its not a problem. Mostly blocking decorator is linked with an issue label as shown below:
issue:(blocking): --> "Description of what is is broken".
Non Blocking: Non Blocking comments are something which need not be fixed or corrected. I give non blocking comments when something is incorrect but it wont impact feature/task.
Expected Outcome: It is ok if the author does not take any action on non blocking comments. non-blocking decorator is usually linked with an nitpick or suggestion label
Ex: nitpick:(non-blocking): The comments is copied from the previous function. Could be changed.
If Minor: I use this decoration in case I see a possibility to refactor some code only if its a minor change.
Expected Outcome: Author can optionally refactor the code if time permits. Not doing so is fine too as long as there is a ticket to perform this refactoring if it makes sense.
Mostly if minor decorator is linked with an thoughts/suggestion label
suggestion:(if-minor): This function can be refactored. Please note that the current code works fine and meets the task/feature requirement.
If aligned: Lets say I find a scenario which no one has thought about in the requirement * if the author implements this after aligning with the stake holders, it may help the end user.
Mostly if aligned decorator is linked with an thoughts/suggestion label
Expected Outcome: The author could probably talk to the stake holders, align and implement the requested change. Its is quite ok to skip it as well if there is not much time to do this change along with this task.
if-aligned ( non-blocking ) : What are your thoughts on adding a confirmation prompt before performing this resource intensive activity? It may save time for our users and keep our resource consumption lower.
A combination of labels and decorators convey the right intent. Here are some more examples:
suggestion:(non-blocking): This line of code works well and meets our requirement. What are you thoughts on considering ?. operator for the same.
nitpick:(non-blocking): "What do you think if we name this variable as X instead of Y."
What I would like to improve
Speed - At times I would take a bit more to complete code reviews but I try to be thorough. Luckily I've a very supportive team.
Always finish the review within 24 hours after its assigned. This only becomes a challenge when I'm working on a bigger task and the code review request is also for a equally bigger task.
Thank You
If you have reached so far, congratulations. Do you do things any differently, please share in the comments.
Until Next time!
Photo by Oğuzhan Akdoğan
