Comments are code smells

Developers are typically encouraged to make sure their code is well commented. Over the years I've seen (and written) some terrible comments that add zero value to the codebase. This happens when the developer doesn't fully understand why we write comments. The aim of this post is to change your mentality towards inline comments - and to encourage you to try not to write them. There is an alternative.

TL;DR – Instead of writing inline comments, see if you can abstract code into a method instead.

Three of the most common mistakes I see when people write comments are:

  • Translating code into English
  • Writing a comment for every line of code
  • Separating/grouping logic in a method by using comments
  • Duplicating information

I consider these to be code smells:

A code smell is a surface indication that usually corresponds to a deeper problem in the system. – Martin Fowler

How can comments correspond to a deeper problem in the system I hear you cry? To explain this, we need to look at the different types of comments and what purpose they serve.

Two types of comments

There are 2 types of comments in my eyes: DocBlocks and inline comments. DocBlocks are the multi-line comments found above your function/method/class and provides verbose information about it, such as: what it does, why and the input & output. They look a little like this:

/**
 * Remove all images and re-add them
 * @param  array  $images
 * @return array
 */
public function syncImages($images)
{
    // ...
}

DocBlocks are important, especially if your using them to auto-generate documentation. However, if you’re not generating docs then consider not writing them for every method. With good method name and typehinting we can often eliminate the need for them; or at least reduce duplicate information. Here’s how I would re-write the snippet above:

/**
 * Remove all images and re-add them
 */
public function syncImages(array $images): array
{
    // ...
}

Here I’ve added typehints for the input and output, which negates the need for that information in the DocBlock as they are self documenting. I decided to keep the method description in this case to provide more clarity on what syncing means.

Generally speaking, you can’t go too far wrong writing DocBlocks. Some tips:

  • Don’t add a description if you are only repeating the method name
  • Add @params and @returns when you are unable to typehint. Consider alternative design patterns which would allow typehinting
  • Add @params and @returns when you need to provide a description for a variable

Inline comments

If DocBlocks are found outside your method, inline comments are found inside it. They are typically single line comments but can span multiple lines too. Here’s an example method that validates an image upload (before it hits a controller), with inline comments:

public function handle($request, $next)
{
    // Check image file size

    $maxFilesize = 10000000000;

    // Get the size of the image
    $imageSize = $request->header('X-Upload-Content-Length');

    // Make sure the uploaded image isn't larger than the allowed limit
    if ($imageSize > $maxFilesize) {
        throw new RequestEntityTooLargeException();
    }


    // Check image file type

    $supportedFormats = ['image/jpeg', 'image/png'];

    // Get the type of file that's being uploaded
    $contentType = $this->request->header('X-Upload-Content-Type');

    // Make sure we support that file type
    if (!in_array($contentType, $supportedFormats)) {
        throw new UnsupportedMediaTypeException($contentType);
    }

    return $next($request);
}

It’s pretty easy to follow through the code if you need to, plus there are comments to help guide you through it. Some people might argue that this is fine, but I think we can do better. If we are treating inline comments as code smells, then this method stinks! We should therefore stop and think to ourselves: “Is there a better way to write this to remove the code smell?” – The answer is usually yes. Let’s look at how we might go about doing that.

Removing inline comments

As with all best practices with development, it boils down to making our code easier to change in the future. That is also why we write comments. To make it easier to anyone to work on (i.e. change) the project – or for us coming back to the project later down the line when we need to make changes.

So, if you’re writing an inline comment it should serve the purpose of making that code easier to change. They should explain the reasoning why the code is the way it is, such as for a particular business requirement or due to some weird behaviour of some other piece of code. But there is another way to achieve this goal without writing inline comments.

Write methods, not comments

Treat inline comments as a code smell. When you catch yourself writing one, stop and think about an alternative way of writing your code to eliminate them. Most of the time there is a better way. The technique I use is to abstract the code into its own method. By doing so, you can:

  • Use DocBlocks to describe your behaviour and reasoning (or reference the Stack Overflow answer you were ‘inspired’ by)
  • Give it a descriptive name to make your code read like English. This stops you trying to translate code into English in comments as your code is already in English
  • Give each method a single responsibility

The outcome is that you will have a lot more methods – often only 1 or 2 lines long. This comes with a handful of hidden benefits too: Debugging is slightly easier as your descriptive method name will appear in the stack trace. As you’ve isolated your code, it can sometimes make it more reusable and easier to unit test. You can dive into what the code is doing only when you care about it. All of these lead to making your code easier to change.

Below, I’ve rewritten the example from earlier to replace inline comments with methods:

public function handle($request, $next)
{
    $this->request = $request;

    $this->guardAgainstUnsupportedMimeTypes();
    $this->guardAgainstLargeImages();

    return $next($request);
}

private function guardAgainstUnsupportedMimeTypes()
{
    $supportedFormats = ['image/jpeg', 'image/png'];

    $contentType = $this->request->header('X-Upload-Content-Type');

    if (!in_array($contentType, $supportedFormats)) {
        throw new UnsupportedMediaTypeException($contentType);
    }
}

private function guardAgainstLargeImages()
{
    $maxFilesize = 10000000000;
    $imageSize = $this->request->header('X-Upload-Content-Length');

    if ($imageSize > $maxFilesize) {
        throw new RequestEntityTooLargeException();
    }
}

Now, the handle() method is very easy to read (and importantly, scan). You can clearly see it prevents unsupported file types and large images. Without the need for a single comment. And if you care about how it goes about doing that, you can jump to the specific method and find out.

In this next example, we will look at simplifying a controller responsible for getting (and filtering) orders on an api.

/**
 * Display a listing of the resource.
 *
 * @param  Request $request
 * @return array
 */
public function index(Request $request)
{
    // Creates a query builder for an order
    $orderQuery = Order::whereRaw('1=1');

    // Filter orders by their status (e.g. completed)
    if (request()->has('status')) {
        $orderQuery->where('status', request()->input('status'));
    }

    // Filter orders by customer
    if (!request()->has('customer_id')) {
        $orderQuery->forCustomer(request()->input('customer_id'));
    }

    // Converts a collection of orders to the right structure for the api
    return fractal($orderQuery->get(), new OrderTransformer)->toArray();
}

The first line is a hack around Laravel’s query builder, which allows us to build up a chainable query. Next we check if we need to filter by status and/or customer before preparing the response for the api. All pretty straight forward, but lets look at an alternative way of writing this:

public function index(Request $request): array
{
    $orderQuery = $this->createOrderQuery();

    $orderQuery = $this->filterByStatus($orderQuery);
    $orderQuery = $this->filterByCustomer($orderQuery);

    // Converts a collection of orders to the right structure for the api
    return fractal($orderQuery->get(), new OrderTransformer)->toArray();
}

private function createOrderQuery(): Builder
{
    return Order::whereRaw('1=1');
}

/**
 * Filter orders by status (e.g. awaiting_payment, batched, completed)
 */
private function filterByStatus(Builder $query): Builder
{
    if (!request()->has('status')) {
        return $query;
    }

    return $query->where('status', request()->input('status'));
}

private function filterByCustomer(Builder $query): Builder
{
    if (!request()->has('customer_id')) {
        return $query;
    }

    return $query->forCustomer(request()->input('customer_id'));
}

Firstly we have removed DocBlock from the index method in favour of typehinting. Also because we are using a known naming convention we can remove the description too.

The logic of how we create the query builder has been moved into its own method. This allows me to reuse that logic for other parts of my api if needed, while keeping it easy to refactor if I find a better way than this hack. It also eliminated the need for my comment as the method name createOrderQuery is pretty self explanatory. Thanks to typehinting I know I will get an instance of Eloquent’s Builder back.

We have also abstracted the filtering into two new methods: filterByStatus and filterByCustomer. By doing so, the index method is much sorter and easier to digest. If we need to filter another resource on our api by customer, we can easily move filterByCustomer into a trait (e.g. CanFilterCustomers). Note that I kept the description on the filterByStatus method as I think it adds value to give examples of statuses.

Conclusion

Challenge yourself to think of alternative ways to make your code easier to change. Treat inline comments as code smells. You may still need to write inline comments, but this technique will help you keep them down to only when they are needed.

Remember, only stop writing comments in favour of an alternative approach. Don’t just stop writing comments; they are better than nothing.