Name Smells

I would imagine that most developers reading have heard of code smells. I’ve also seen various references to other concepts such as design smells (which are probably similar to code smells) or process smells. Generally, when you read these, you chuckle or nod in agreement, thinking that you’ve seen them before. For those not familiar with these concepts, the idea is there are certain characteristics of a code base that tend to indicate deeper problems. That is, the indicator may or may not be a problem in and of itself, but its presence usually correlates with code that is difficult to maintain, understand, or extend.

Today, I’m going to propose a concept called “name smells.” I fully acknowledge that these could simply be referred to as a subset of code smells, but I think they deserve their own terminology due to specificity. What I’m referring to is names of methods or classes that raise my hackles in Pavlovian response to repeated unpleasant associations. With code smells, you might see a lot of obviously copied and pasted code and think, “Uh oh.” With name smells, you don’t even need to inspect the code — you can just browse through the directory or project listing (for class names) or the object graph (for method names).

These smells are specifically for object-oriented development environments and are probably best applied in Java or C#. So, without further ado:

1. Utils in a Class Name

As someone who buys in fully to the OOP concept when working in OOP languages, this one makes me shudder. Because I know that what I’ll see if I open the class in my IDE is a static class chock full of a free-floating hodgepodge of methods with divergent responsibilities. This class will probably be long when you first encounter it. After every development phase, it will probably be even longer. Eventually, it may turn into two or more static classes, all ending in Utils. It might even make its way into different namespaces or assemblies.

The underlying problem that this smell is an indicator of is functionality not associated with any object. In OOP, this is a no-no. The existence of this class generally means that there is a substantial amount of functionality in your application that hasn’t been properly considered. The ramifications of this are that a great deal of “tribal knowledge” is required to write code. Consider something like:

class Customer
{
    string _customerNumber;
    public void Populate()
    {
        int x = CustomerUtils.GetCustomerContext().GetFirstAvailableCustomerNumber();
        _customerNumber = StringUtils.ConvertIntToValidCustomerNumber(x);
    }
}

Now, let’s say I’m new to this project, and I need to create a new kind of customer. Given the way the code there looks, it’s a safe bet that somebody is going to tell me not to inherit from Customer. So, I create NewCustomer, and I need to figure out how to set the customer number property. Without inspecting the source code, how on Earth do I do this?

I need to know of the existence of CustomerUtils and that it has some static method that gets me a customer number (via a law of demeter violation, no less). Now, that returns an int, but I need a string. So I also have to know that some other utils class is responsible for converting that int to the string that I want. Realistically, I won’t know this because it makes no sense. I have to go to one of the team members that wrote this stuff in the first place and pick their brains for tribal knowledge.

This is an extreme example perhaps, but it gets the point across. If there are static utils classes floating around, you have to know about the static classes and what their methods are to do things. If these static utils methods were pulled out of static purgatory and put into some instance object called “CustomerNumberGenerator,” that would make more sense to me. I’d be looking through the classes in the code base, wondering how to get a customer number, and I’d see a class that practically says, “ooh, ooh–instantiate me!” In the static utils world, I have to say, “CustomerUtils. Maybe if I dig through that I’ll find something. No, nothing… oh, wait. One of these methods returns something called a CustomerContext, maybe I should look at that.”

The irony is that Utils classes are supposed to be handy, but they bloat to such a degree that everyone will avoid digging into the code bowels if at all possible.

2. Helper in a Class Name

This is similar to the previous one, but with an important distinction. In the event that Helper is a static class, see the previous section because a Utils class by any other name smells just as badly. The distinction is that this is often an instance object, and I think of this as a different smell. It seems to indicate that the previous object was not up to its task and it needs ‘help.’ The class Tuna just isn’t that good on its own, so along comes instance class “TunaHelper” to make the original class more delicious.

I contend that the existence of TunaHelper or any other Helper is a name smell. Why do your classes need help? Being philosophical for a moment, we might say that all classes need help in an application unless the application is so tiny as to fit reasonably within a single class. That is, if you’re modeling an Engine, you could theoretically call the Car that contains it “EngineHelper.” If that’s what’s happening in the helper classes, the good news is that the smell is superficial and indicates only that the designer is bad at labeling things.

But if bad naming isn’t the issue, then you have a bona fide smell. What you probably have is poor class cohesion in your object model, meaning that things aren’t grouped in such a way that related functionality occurs together. If I have my Engine instance, and then I also have an “EngineHelper” instance, what we’re doing is essentially spreading the functionality of Engine across two different classes. If you have Foo and FooHelper in general, it’s pretty likely that anytime Foo needs to change, FooHelper also needs to change and vice versa. You have tight coupling resulting from your low degree of cohesion.

The code could probably benefit from merging Foo and FooHelper and, if the result is too large, critically examining how Foo might be broken down into more manageable components. But, generally speaking, anytime I see a FooHelper, be it an EngineHelper, a TunaHelper, or any other Helper, my immediate instinctive reaction is to think that the Helper descriptor is probably a piece of bitter irony and to cringe at the thought of opening the Helper and the Helpee to see what sort of ad-hoc maze of code exists within.

3. Manager in a Class Name

This has some degree of similarity to the Helper, though I’d say it’s more specific and that it indicates a subtly different problem. Again, this depends to some degree on the conventions of those doing the naming, but usually when I see “manager” it has a subtly different implication than “helper.” Many of the “managers” I’ve seen “manage” their charge by knowing an inappropriate amount of detail about its inner workings.

The result is fairly similar in that we get tight coupling, but whereas the helper probably exists to provide some different, if dubious, functionality, the manager usually exists as spillover from a class that is growing like a late 1990s baseball slugger. At around line 2000, the class author says, “Man, this is too big for a single class. I’ll refactor it into two classes: Foo and FooManager.” Then some particular division of labor exists where the manager is given various aggregation and collection responsibilities of the original class while the managee is given the leftovers. But since this is really a single class in the eyes of the creator, the now separated classes know all about the internal workings of one another.

The reason I say that this is a name smell is that you generally see the name “Manager” in a class as part of an effort to deodorize a code smell like “giant method” or “giant class.” The result is that you’re just trading one code smell for another–shuffling the deck chairs on the Titanic as it sinks, without stopping to address the root problem.

4. Initialize() Method

This is a name smell in a method. If I see this as a class method–especially a private class method–I know that I’m about to see something ugly. Generally, this method comes into existence when someone looks at their class constructor, realizes that it has grown to be 50 lines long, and thinks that’s ugly. Abstraction to the rescue! We’ll factor it out to a different method, bury it at the bottom of the file with the privates, and now our constructor is nice and clean again.

Except that it’s not. At least with the cluttered constructor the badness was advertised right up front instead of being tucked away somewhere. There’s no reason for constructors to be doing all sorts of stuff. That’s a code smell. Usually, it means that on instantiation, your object is designing, building, and installing the kitchen sink. It’s also a strong indicator of a God Object.

Objects with injected dependencies or simple initialization logic do not need an abstracted “Initialize()” method. So I maintain that if you see one in a clean constructor, things are about to get ugly:

class Foo
{
    public Foo()
    {
        Initialize(); //Run away, fast.
    }

    ...

    private void Initialize()
    {
        //Because the smell above is coming from the baked roadkill here
    }
}

5. Instance in a Method or Property Name

This is often a property instead of a method, but this name smell correlates heavily with the smell of singletons in your code. I might make a post in the future on singletons, so I won’t do it here. But I also might not, since the singleton issue has been pretty much beaten to death, with most recognizing them as a code smell.  Those who don’t recognize them as such have probably heard all of the arguments and have not found them persuasive.

6. “Type” Pretty Much Anywhere (that isn’t reflection)

If you’re an OOP purist, this is usually an indicator of missed opportunities for polymorphism. When you see the word “type” in enums, methods, classes, etc., it usually means that the author is simulating polymorphism with its procedural equivalent.

That is, the author has given a class some property called “type,” probably assigned an enum to back it, and subsequently demanded that clients of his code switch over its “type” to know how to behave:

enum CarType { Sedan, Coupe, SUV };

class Car
{
    public CarType Type { get; set; }
    ....
}

class ClientOfCar
{
    public void DoSomethingWithCar(Car car)
    {
        switch(car.Type)
        {
            case CarType.Sedan: ....
        }
    }
}

So, if you dig into a class and see that it has a “type” property or a method that mentions its “type,” and we’re not talking reflection, you’re probably going to see redundant case statements strewn about the application, representing the smell of duplication.

Final Thoughts

Everything here, is, of course, subjective. The notion of “smells” in the first place discusses something that is often an indicator of underlying problems. That means that you may full well have a “Manager” class or an “Initialize” method that is perfectly reasonable and is not an indicator of any problems. In addition, this is based on my own experience of looking at a variety of code bases. Over the course of time, I have noticed a high correlation between these words and code that is very procedural and/or difficult to maintain/modify/understand. Your mileage may vary.