Demystifying The Code

Helper Classes Are A Code Smell

I recently made one of those sweeping statements to some of my colleagues that almost always come back to bite you on the …: It was something to the effect of: "Having the word ‘Helper’ in a class name is a code smell." A pretty big debate ensued and I challenged them (and you) to give me a good example where it is appropriate (outside of testing classes).

So, why do I think it is a code smell (aside: I’m actually getting really tired of the term code smell)? It seems that all "helper" classes violate at least one of the two following principles / practices: 1) SRP and/or 2) Good Naming. Let’s take a look at these one at a time.

SRP

Hopefully, you are familiar with the "S" in Uncle Bob’s SOLID Principles of Object Oriented Design. If not, you have some great reading ahead of you: Clean Code: A Handbook of Agile Software Craftsmanship and Agile Principles, Patterns, and Practices in C#.  Uncle Bob’s definition of SRP is: "There should never be more than one reason for a class to change". I think of SRP as saying that a class should do one thing and do it well (and as a result should be testable).

Most often, when I see a "Helper" class, and I’ve seen a lot of them, they are usually classes jam packed with a host of differing and unrelated utility methods. The motivation for naming them {Something}Helper is, generally, that the class does a variety of things and it is therefore impossible to give it a distinct name. That is a clear violation of SRP.

How can you tell if your class violates SRP? See if you can describe what your class does in one sentence. Were you able to do it? Did your sentence have an "and" or an "or" in it? If you cannot describe what your class does in one sentence without using "and" or "or", it is a good indicator that your class is doing too much. Most helpers require generous amounts of "ands" and "ors" to describe their functionality.

Good Naming

I cannot emphasize enough the importance of well named classes, properties and methods in your code. Names that describe the functionality and purpose of your code make that code understandable and maintainable by others (and yourself when you are reading it in 3 months). Names that don’t reflect the purpose (like Helper) force you or others to read the code in the class (or method) to understand it’s purpose.

Naming classes appropriately is hard. If a class does more than one thing, it is nearly impossible. The inability to come up with a good name for a class or method is, again, a good indicator of this.

An Example

Take, for example, the omnipresent "XmlHelper" class. Yes, I’m sure you’ve seen one or two in your time. This flavour of helper class might have some methods that do the following:

    • Read Xml and return an object like Customer, Order or Trade
    • Serialize objects like Customer, Order or Trade to Xml
    • Parse Xml and return certain element or attribute values
    • Validates Xml against a schema

Logically, if the class does more than one thing, it violates SRP. I would argue that the name XmlHelper is inadequate. It does not tell the developer what it does. Consider the name XmlHelper vs. TradeSerializer or TradeDeserializer. Which one tells you more about the purpose of the class? Are you trying to help Xml?

What is the Big Deal?

So we have broken a design principle or two. What is the big deal? It may not seem to be all that bad. But it is!

Breaking SRP yields unmaintainable code. Suppose that your XmlHelper (among other things) contained code to read Xml and return a Customer. Now suppose that the Customer schema changed (as they do). Where does that code live? You might know if you wrote the helper… if you remembered that it was sitting next to some code that validated Trade Xml and parsed attribute values from an Order.

Now suppose that, instead of the schema changing, the service serving you customers now is serving Json. If you had followed SRP and used dependency injection appropriately, you likely would have had something like an ICustomerDeserializer and an XmlCustomerDeserializer (or maybe just a CustomerDeserializer) that implemented it. You could now create a JsonCustomerDeserializer and use that instead (just a simple change in your IoCContainer).  In either case, the change was isolated.

What about the naming? XmlHelper tells, at most, half the story… that the class has something to do with Xml. But what? This isn’t a mystery novel. The name of the class should tell you exactly what the single purpose of the class is.

Speak Your Mind

Tell us what you're thinking...
and oh, if you want a pic to show with your comment, go get a gravatar!

Demystifying The Code