Comments during Code Reviews

Why do we perform code reviews?

• To learn from other engineers?
• To correct a potential multibillion Naira mistake?
• To point out areas in the PR that can be improved upon to meet best practices, correct a possible design flaw, correct a business logic etc.

Whatever your reasons are from the above, we get to write comments on PRs every now and then. I have worked in one of the biggest software engineering companies in Nigeria in terms of work force (shopKonga June-Nov 2017) and I currently work for one of the best software engineering companies in Nigeria in terms of process and quality (Cotta & Cush) as well and I have seen lots of comments from different software engineers at different levels and roles. I would share both the ones I think are good and bad, why they are good and bad and why you should adopt writing comments the good way.

• don’t use ArrayDataProvider

Seriously!! Why would you drop a comment like this????

A badass guy probably would quickly see reasons why not to use ArrayDataProvider, but for the love of God, always state your reasons why you think something should not be used.

A better way would have been:

I suggest you use CrazyDataProvider instead of ArrayDataProvider because it helps to parse crazy data better which the ArrayDataProvider doesn’t do.

• use a more descriptive name for the intent

do I in any way look like your errand boy! Adding a please in front wouldn’t kill you, would it? PR courtesy.

• Remove header

again!!! you are not writing a commit message.

• This is fine but not needed, swift can infer that the type is a Bool, same for current_position above

Good comment.

• What happens if this fails?

Nice comment.

• Use ternary operator

Is this a military regime? what happened to the word ‘please’ in front or a even better way would have been:

Please use a ternary comment, it makes the code neater and easier to read

• You can use a ternary operation here, makes it look neater

Perfect! I just proposed this above, we are getting there.

• Indenting seems to be off by one space here

Good comment, a better version would be:

Indenting seems to be off by one space here, can you confirm?

• I think this can be put in the strings.xml file

Good comment.

• Can we move this imageUtils to the android commons library?

Yes we can. Good comment!

• ButterKnife.findById has been deprecated and will be removed in the next version of ButterKnife v9. The normal findViewById can be used instead. https://stackoverflow.com/questions/45516753/butterknife-findbyid-method-is-deprecated-in-version-8-8-0

Best I have seen so far, I love this one.

• I think i like C#. await as in duro de right?

Is this a bad comment? what do you think?

• I am assuming this image is for a thumbnail for the app or something like that. You might want to remove the “Activate Windows” watermark. On the contrary, because it is named like with a ticket number, if might have been added by an error. Please do well to remove if that is the case.

Very very good way to write a comment.

So generally, we can categorize comments into three groups:

• Comments as QUESTIONS.
• Comments as SUGGESTIONS.
• Comments as APPRECIATIONS.

Proposed comment structure for all three groups:

1. Comments as QUESTIONS:

Don’t ask vague questions, be very direct:

• Can you explain what this does?
• What happens if this doesn’t get saved? Does it throw an exception or fails silently?

2. Comments as SUGGESTIONS:

State your suggestion and give reasons why you think it should be changed. Don’t give commands, you are not a soldier, and if you are, well, don’t give commands during code reviews.

• I suggest you use an ArrayHelper getValue method here because of its error handling capability instead of accessing the value directly

You could even go further by giving an example:

$a = $b[‘key’]; would raise an error if key is not set but $a = ArrayHelper::getValue($b, ‘key’); would return a null value if key is not set.

• I suggest you reformat this file so it follows PSR-2 coding standard to avoid merge conflicts and adhere to best practices.

3. and finally, Comments as APPRECIATIONS:

Writing code is hard and writing quality code is even harder, it would be nice once in a while to appreciate people’s work.

• Nice work Alice, I really learnt a lot from this.
• Great Job! Bob

you could even merge two or more categories together.

• Nice Job! Alice. I suggest we create an interface for this service so other substitute services can implement the interface as well, this would enable us change to a different service with very minimal efforts when the need arises. What do you think?

You don’t need to follow these rules religiously, of course their exceptions to everything. E.g if you have made a suggestive comment previously explaining why a change should be made, you don’t need to explain on subsequent comments to the same person when you see it again. You could just drop your suggestive comments without explaining.

Writing constructive comments I believe is the hardest part in code reviews but it would help you and your team tremendously in being more collaborative and would go a long way in helping the team achieve success.

These little things matter. Trust me!