Last week I posed a situation:

You come across a line of code that’s obviously wrong, and has no effect at all. It’s been running in production for 5 years.

How would you react?

1. Ignore the non-functional code and move along.
2. Delete the non-functional code.
3. Investigate to try to undertstand the intention, and fix it.
4. Something else.

I’ve come across this exact situation several times recently, in a Go code base. So how did I respond?

TL;DR; I deleted the non-functional code.

But such a short answer is no fun. So let me elaborate on my thought process.

First, I’m absolutely, 100% confident that this code does nothing. Go does not allow side effects for code that simply does assignments, as was the case in the example (i.e. x = x). A number of responses were concerned about test coverage. And I sympathize, but test coverage should not be relevant to such a situation, since the language can offer such solid guarantees. If the code were written in JavaScript, or Perl, or C++, where one could implement side effects, or overload the equality operator or whatever, that’s another story…

Now with that all said, I did consider other options… but ultimately rejected them. Let me go through each of the options I considered, and explain why I skipped them:

1. Ignore the non-functional code and move along.

This would have worked, of course. But this non-functional code was triggering our newly added linter (for good reason). So ignoring the code in the strictest sense wasn’t really a good option. At minimum, I’d have to add a //nolint directive. And I felt this would leave the code worse than before, rather than better or neutral. As-is, the non-functional code just looks like an accident. But as soon as you add a //nolint directive, it looks intentional. Which is probably worse, because now some future reader of the code will be forced to ask why such a strange thing was done intentionally. Of course, I could leave a comment explaining, but that’s not really any better, either. //nolint: Not sure why this code is like this, but I left it this way to punt the burden of making a decision to someone else... Bleh.

That leads to…

3. Investigate to try to undertstand the intention, and fix it.

I actually started down this path. But quickly decided this was actually a risky choice, considering the poor test coverage on this code base. Even if the intention was obvious (it wasn’t), any change I make may change behavior in some way that surprises users. As mentioned, this code has been running for years in production.

So that left me with the option I chose:

2. Delete the non-functional code.

In the interest of cleaning up the code, and reducing the cognitive load of the next person to happen upon it, I decided to invoke the boyscout rule, and delete the code that did nothing. Of course, deleting code from VCS never actually deletes it. If some future coder ever comes upon this function with the intention of implementing the original intention (which I never devined), and thinks “Gee, why isn’t there an x = y here”, they can look at git history and discover that there was in fact a x = x there, and that it was indeed wrong, and should have been deleted… then go back further in history to see where it was added, and probably realize that a simple mistake was made. And they can correct it as appropriate.

Far more likely, nobody will ever have such a question, of course. And by deleting that code, I’ve put the issue to rest forever, and reduced the complexity of our code by a line. Woot!

Of course, it all comes down to a judgement call. And I can easily see good developers making different calls in the same situation.

Now that you see my reasoning, do you agree or disagree?