Commenting code

There are environments in which commenting code is desirable. There are people who like to add docblocks to every method. I am clearly not one of them. I like to keep them to a minimum and use methods native to the programming language to achieve the same goal (like strict typing or return type hints).

And then there are the comments that describe why the code was made in a given way. More often than not those are the cases when you cannot refactor the code to be more descriptive, so there’s the urge to just leave two lines of comments and be done with it.

I recently had a similar situation. After making the following change, I wanted to describe the reasoning behind it in a comment:

TaskCollectionVisitPrefetcher.diff

public function findUpcomingForPatient(Patient $patient): ?Visit
{
-    return $this->cache->fetch($patient->getId()) 
-        ?: $this->visits->findUpcomingForPatient($patient);
+    $key = $patient->getId();
+    if ($this->cache->contains($key)) {
+        return $this->cache->fetch($key);
+    }
+
+    return $this->visits->findUpcomingForPatient($patient);
}

The change might be obvious to some, but at the same time the reasoning might be lost to others, and they might want to refactor it back to the previous format — to keep it simpler, or to make a micro-optimization by avoiding the extra call to the cache, and limit the risk of a race at the same time.

I started to comment and then I thought of another way. I ended up writing a test for this scenario:

TaskCollectionVisitPrefetcherTest.php

public function repository call is prevented if the cache contains a null value()
{
    // given …
    // when:
    $sut->prefetch($this->task);
    // then:
    $visits
        ->expects(self::never())
        ->method('findUpcomingForPatient');

    $sut->findUpcomingForPatient($this->patient);
}

There are a few benefits of this solution:

1. There is more than just a comment holding back future devs from creating a regression here. They might be tempted to refactor, but they will have to fulfill this requirement the tests impose on them
2. I actually have higher confidence that the feature works as expected thanks to the automated test
3. The code got refactored a bit in the process (for example, I noticed that one of the dependencies required a concrete implementation instead of the interface)

And the original message I wanted to leave in the comments: it’s all there, in the test name & its setup. The original method was annotated with @see tag for this issue to be more discoverable.

And by the way, this change fixed some nasty n+1 queries problem that really affected our load times for some scenarios, where items number grew from standard page size of 30 to over 600. And it made a difference.

Before:

After:

The takeaway? Usually, there is a better way than just leaving a comment in code.