Code Review Good Practice

Have you ever encountered the following situation during a code review? One of your teammates, say Jimmy wrote this comment on one part of your code:

I like this code block. I think it does the job and reduce the complexity of the code. I think this line also pretty us of … It’s pretty elegant, but I would try a minor change to this in the future… I would keep this as it is… How about the XXX functionality? Have you considered backward compatibility? … Is this an expendable code for other feature? … I do like it, because the tests are… blablabla…

Sometimes, people ramble! So what exactly Jimmy wants? Does he want a correction and next rounds of review or does he just want to express his opinions, or maybe he just really likes the code? No one knows… Probably not even Jimmy himself. How could we avoid this? I will talk about a good code review practice that I have learned during my short software development career.

Summarize Intention With a Mark

I won’t be talking about how to make a good comment and what to consider when review codes. There are tons of documents out there that people can reference to. Instead, I want to talk about how to make the person who views your comment understand your intention. Basically, the idea is the add a punctuation mark before your comment to let people know your general intention. The practice that my team and I use is:

[-], [+], [?], [!]

What does this mean? Maybe let me show you with some examples.

[-] Suggest Minor Change (Low Priority)

When putting a [-] on front of the comment, we are telling the author that this is a minor issue. It is usually a code style issue, bad naming, typo and extra spaces etc… This comment suggests changing it, but opening another revision would take few hours or days for the next review. To save time, minor issues can be changed in the next MR or CR or whatever you call it.

Here is an example of the [-] comment:

[+] Strongly Suggest to Change (High Priority)

[+] is usually used when there is a strong suggestion to change the current code. Usually it’s about maintainability, expandability or design flaws. The code is probably working, but it might add difficulties to testing or did not use a good design pattern. One example of the [+] comment would be:

[!] Flags, Need to Change (Top Priority)

[!] is usually a serious issue! It means that if this code is shipped, the program will be broken. It’s usually a miss by the author due to undertesting. Maybe some corner cases are missed and the reviewer saw it. For example:

[?] Confusion, Asking for More Explanation (Medium Priority)

When a comment starts with [?], it usually means that the reviewer does not understand the purpose of this line or block of code. It could be your mistake but also it could be that the reviewer is missing some contexts around what you are doing. In this case, the reviewer is not suggesting a change but seeking for more explanation. It’s a learning opportunity for the reviewers as well. For example:

Conclusion

I hope this can help the early career software engineers to clearly express their intention when writing comments in a code review. This is not a guideline on how to do code reviews but simply sharing something that I found helpful during my work.

Written on December 25th, 2021 by Ted Wang