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:

<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;”>// Get all users.
userService.getAllUsers();</pre>

</td>

<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;”>// Check if the name is empty.
if (name.isEmpty()) { … }</pre>

</td>

</tr>

</tbody>

</table>