Code Smells are signs, not problems

The concept of Code Smells seems to be somewhat misunderstood.

In particular, the claim that "Comments are a Code Smell" seems to be an extremely dangerous one to make. If you espouse this position, you are liable to be called unprofessional, or an idiot.

Even el reg seems to have gotten the wrong end of the stick, and claims that to declare comments to be a smell is to "spoil a good guideline by taking it to extremes".

It's not a smell!!!!1!!11one1!

Having questioned your abilities (and possibly the virtues of your mother), some responders go on to angrily declare that the only problem is in comments that describe what the code does, and that comments should state why, not what.

Others may assert that sometimes you need commentary to explain a counterintuitive approach forced upon you by an underlying API.

Others still, may claim that they have been struggling with some tangled mess of illegible code, that could have seriously benefited from commentary.

This post on programmers.stackexchange.com, engendered some responses of the kind I describe.

The problem is that these responses never argue against the assertion that "comments are a code smell", but against the assertion (which nobody has made), that comments are an antipattern.

The common feature of those who argue against the assertion that "Comments are a Code Smell", is that (as in this question) they seem to interpret "Code Smell" as a synonym for "Antipattern". Often, they then go on to provide proof that comments are, indeed, a code smell.

What is a code smell?

Here are some definitions of a code smell:

Wikipedia:

"In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem."

WardsWiki (where the term was first coined):

"A code smell is a hint that something has gone wrong somewhere in your code. ... Note that a CodeSmell is a hint that something might be wrong, not a certainty."

Pay attention to the emphasised words in the WardsWiki quote. Basically, code smells are symptoms, not diseases. If I have a runny nose, it could be because I have a cold, or I could have just been surfing, and the nasal enema that comes with a wipeout is working its magic. Any number of things could be the cause, but not all of them require treatment.

Rebuttals rebutted

Considering smells as hints, let's revisit the "It's not a smell!!" responses from above, and see how each instance proves that comments are, indeed, a smell:

Specific Smell:

There are comments describing what the code does.

Implies:

The author of the comments has a poor understanding of the programming language.

The deeper problem:

One of your programmers is incompetent

What may be wrong:

The incompetent programmer has probably introduced diverse bugs.

Why it might not be wrong:

The code might have been written as part of a course - either by a student who needs to demonstrate they can read code, or by an instructor to describe the code to some neophytes.

Specific Smell:

There are comments explaining why some code does what it does.

Implies:

Your motives are non-obvious.

The deeper problem:

Your motives are arcane and unnecessary.

What may be wrong:

Your incorrect motives have led to some incorrect or unnecessary code.

Why it might not be wrong:

The code is necessarily complex and confusing - complicated mathematics (for example) can be difficult to explain without additional commentary. There may be an accepted domain-specific convention that is being followed, that renders the code obvious to a Subject Matter Expert, but would appear counterintuitive to an experienced software developer from a different subject.

Specific Smell:

You have to comment around the foibles of a 3rd party API

Implies:

The API is counterintuitive

The deeper problem:

Counterintuitive APIs make for a potentially fragile system.

What may be wrong:

Somewhere in the code, the API's conventions may not have been correctly followed.

Why it might not be wrong:

The counterintuitive API may be the best, or only, fit for the job.

Specific Smell:

Some uncommented code was hard to understand.

Implies:

The code needs comments to be understandable.

The deeper problem:

Code that can't easily be understood without comments is more likely to suffer a disconnect between code and comments, or be just plain wrong.

What may be wrong:

Comments may not reflect the code, developers will be reluctant to touch it, cargo-cult programming may arise. Code that is hard to understand is easy to break and scary to change; regardless of how much commentary it contains.

Also, even a developer who diligently updates the comments whenever he updates the code, may misunderstand the side-effects of a change (or quite likely, trust an incorrect comment elsewhere), and update comments incorrectly.

Why it might not be wrong:

If you are programming in Assembly or Brainfuck, the only scope you have for increasing the readability is by commenting.

Note that if you are adding commentary to some spaghetti code to make sense of it before refactoring, then that is still something wrong.

Anyone who still asserts that comments are not a code smell, please give me some examples of non-smelly comments, and I'll see if I can add them to the list.

Stack overflow at line 0.

Recursive DOM methods in Internet Explorer can cause a stack overflow error message to appear, even at a relatively shallow depth. This is an exploration of the problem, and how to avoid it.

The Problem:

Some time ago, developing ubiquity xforms, a large client side javascript library, we ran into a problem. On Internet Explorer, we would frequently see the message "Stack overflow at line: 0". The message always appeared after the completion of some deeply recursive function, not during, and the message did not correspond to any erroneous behaviour. The code seemed to execute correctly, wasn't prematurely aborted, and didn't seem to be displaying any kind of unpredictable behaviour.

We found no helpful information about this message, Google returned many instances, on fora and mailing list archives, of unfortunates asking "why am I getting this error message?". The answer was always either "because you have a stack overflow", or STFW, pointing to responses of the former variety.

In a time of tight deadlines, there was an obvious "fix": cut out the recursion, so we did just that. Where plausible, we flattened the recursion out into a loop, or used window.setTimeout with a zero timeout period to "reset the stack".

Sadly, more situations started to crop up where these options were not applicable, and working around it was proving more and more difficult. Not to mention the fact that the recursiveness was sometimes user-defined and therefore effectively out of our control. So, I resolved to investigate the problem properly, in the hope of finding a genuine solution. Which I did.

Simple recursion:

A simple recursive function, such as:

function recurse(n) {
 if (n) {
     return recurse(n-1) + " " + n;
 }
 return "numbers:";
}

called thus: recurse(3), it will return "numbers: 1 2 3". This can successfully be called with much larger arguments (e.g. 257, 2049), returning the appropriate result without error message. When the number gets too big (1048576 was when it broke for me), it reports a stack overflow with a meaningful line number and stops working.

Recursive methods:

An object defined thus:

{
recurse:function(n){
if (n) {
return this.recurse(n-1) + " " + n;
}
return "object: ";
}
}

Also works as expected, with no stack overflow message until the stack genuinely overflows.

Recursive DOM methods:

Ubiquity XForms decorates DOM elements with members of a given object, essentially extending the DOM so that it can deal with the specifics of XForms elements, as well as HTML elements. As such, most functions are called as methods on DOM objects.

Once the document is loaded, adding the following method allows this to be tested:

document.body.recurse = function(n) {
if (n) {
return this.recurse(n-1) + " " + n;
}
return this.nodeName + ":";
}

As before, calling document.body.recurse(3), works as expected, returning "BODY: 1 2 3". As does document.body.recurse(13). However, although document.body.recurse(14) returns the expected value, it also displays the error message "Stack overflow at line 0". The root cause, therefore, appears to be that in IE, The stack for calling DOM member functions is mere 13 deep.

The Cure:

Since the problem only occurs when calling functions as members of DOM objects, it can be resolved by removing the recursiveness to a non-member function. Instead of defining document.body.recurse as above, it may be defined thus:

var recurse = function (self, n) {
if (n) {
return recurse(self, n-1) + " " + n;
}
return self.nodeName + ":";
}

document.body.recurse = function(n) {
return recurse(this,n);
}

By passing "self" explicitly as a parameter, rather than using the built in "this" keyword, document.body.recurse defined in this fashion, it may be called with higher values than 14, without engendering the stack overflow message.

Another possibility is to define recursive methods as members-of-members of DOM objects, thus:

document.body.recursor = {
self:document.body,
recurse: function(n) {
if (n) {
return this.recurse(n-1) + " " + n;
}
return self.nodeName + ":";
}
}
document.body.recurse = function(n) {
return this.recursor.recurse(n);
}


In either case, any code within your library should call the "recursion-safe" version, (recurse/2 or element.recursor.recurse), otherwise the problem will still occur. The element.recurse method is simply to be used as an entry point by clients of your library, or to maintain compatibility with an existing interface.