Code Health: To Comment or Not to Comment?

Tester News Bot

This is another post in our Code Health series. A version of this post originally appeared in Google bathrooms worldwide as a Google Testing on the Toilet episode. You can download a printer-friendly version to display in your office.

By Dori Reuveni and Kevin Bourrillion
While reading code, often there is nothing more helpful than a well-placed comment. However, comments are not always good. Sometimes the need for a comment can be a sign that the code should be refactored.

Use a comment when it is infeasible to make your code self-explanatory. If you think you need a comment to explain what a piece of code does, first try one of the following:

Introduce an explaining variable.

<table class=“my-bordered-table”>

<tbody>

<tr>

<td style=“background-color: #f4cccc; vertical-align: top; width: 50%;”>

<pre style=“background-color: #f4cccc; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Subtract discount from price.
finalPrice = (numItems * itemPrice)
- min(5, numItems) * itemPrice * 0.1;</pre>

</td>

<td style=“background-color: #d9ead3; vertical-align: top; width: 50%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>price = numItems * itemPrice;
discount =
min(5, numItems) * itemPrice * 0.1;
finalPrice = price - discount;</pre>

</td>

</tr>

</tbody>

</table>
Extract a method.

<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #f4cccc; vertical-align: top; width: 50%;”>

<pre style=“background-color: #f4cccc; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Filter offensive words.
for (String word : words) { … }</pre>

</td>

<td style=“background-color: #d9ead3; vertical-align: top; width: 50%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>filterOffensiveWords(words);</pre>

</td>

</tr>

</tbody>

</table>
Use a more descriptive identifier name.

<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #f4cccc; vertical-align: top; width: 50%;”>

<pre style=“background-color: #f4cccc; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>int width = …; // Width in pixels.
</pre>

</td>

<td style=“background-color: #d9ead3; vertical-align: top; width: 50%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>int widthInPixels = …;</pre>

</td>

</tr>

</tbody>

</table>
Add a check in case your code has assumptions.

<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #f4cccc; vertical-align: top; width: 50%;”>

<pre style=“background-color: #f4cccc; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Safe since height is always > 0.
return width / height;</pre>

</td>

<td style=“background-color: #d9ead3; vertical-align: top; width: 50%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>checkArgument(height > 0);
return width / height;
</pre>

</td>

</tr>

</tbody>

</table>

There are cases where a comment can be helpful:

**Reveal your intent: explain why the code does something (as opposed to what it does).

<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #d9ead3; vertical-align: top; width: 100%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Compute once because it’s expensive.</pre>

</td>

</tr>

</tbody>

</table>**
**Protect a well-meaning future editor from mistakenly “fixing” your code. **<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #d9ead3; vertical-align: top; width: 100%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Create a new Foo instance because Foo is not thread-safe.</pre>

</td>

</tr>

</tbody>

</table>****
**Clarification: a question that came up during code review or that readers of the code might have. ****<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #d9ead3; vertical-align: top; width: 100%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Note that order matters because…</pre>

</td>

</tr>

</tbody>

</table>******
Explain your rationale for what looks like a bad software engineering practice. ******<table class=“my-bordered-table” style=“width: 100%;”>

<tbody>

<tr>

<td style=“background-color: #d9ead3; vertical-align: top; width: 100%;”>

<pre style=“background-color: #d9ead3; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>@SuppressWarnings(“unchecked”) // The cast is safe because…</pre>

</td>

</tr>

</tbody>

</table>******

On the other hand, avoid comments that just repeat what the code does. These are just noise:

<tbody>

<tr>

<pre style=“background-color: #f4cccc; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Get all users.
userService.getAllUsers();</pre>

</td>

<pre style=“background-color: #f4cccc; border: 0px; color: black; margin-bottom: 0; margin-top: 0; padding-bottom: 0; padding-top: 0; padding-left: 0;”>// Check if the name is empty.
if (name.isEmpty()) { … }</pre>

</td>

</tr>

</tbody>

</table>