What’s In A Name? Probably the Quality of Your Code.

Name Smells Revisited

A long time ago, I blogged about a concept I dubbed “name smells”. Among other things that I described making me wary of a piece of code just by its name were the words “Helper” and “Manager” in a class name. My issue with this was and is that this tends to be indicative of a class badly coupled to whatever it’s “managing” or “helping”.

Today, I was adding some code to a large, long-standing code base, and I ran the Microsoft code metrics on the large assembly to which I was adding it. I then went looking for my class to see its metrics, for reporting purposes in an implementation document I was constructing. Out of curiosity, I decided to sort the classes by Maintainability Index to see how mine stacked up. I was pleased to see that it had a decent enough index of 88 and was relatively near the top.

From there, I became curious to see which classes were near the bottom, so I scrolled up, since they were in ascending order. As I got up into the 60’s, 50’s, and 40’s, I noticed an unmistakable trend. The worse the metric level, the more frequently I was seeing classes with names like “FooHelper”, “BarManager”, “BazHandler” and “ReBarController” (not to be confused with Model-View-Controller controllers, these ‘controllers’ were not implementing a pattern of any kind). When I scrolled back to the 70+ range, there were exactly 0 classes with names like these. My gut take had been somewhat empirically reinforced, if not outright validated.

Blame the Name?

So, what gives? Does Microsoft look for keywords like those and dock them 20 maintainability points or something? I opened a few of them up for a look and beheld gigantic methods, lots of state flags, and all of the things you might expect in a class with a poor maintainability score. So, Microsoft does not, apparently, share my intrinsic bias against classes with names like these. It must be something else.

Now, obviously I think there’s a correlation between hard-to-maintain classes and these names, or I wouldn’t have blogged about it. But still, you’d think in a pretty large assembly, there would have been one that bucked the trend. The exception to prove the rule….? But no, not a single one.

I spent a bit of time pondering this, glancing back at my old post. When I wrote that, I was thinking that generally what you see is some class evolving to gargantuan proportions, at which time the developer plays Solomon and cleaves the class awkwardly in two with one half of the class being the “Manager” and the other the “Managee”. (I admittedly don’t know exactly how these things get released into the wild, since I don’t do it, myself). But, I don’t think that’s always the case. I think that maybe, sometimes, they start out innocently.

If they start out the result of Solomon the Developer, they’re hosed from the beginning. But, if they start out simply, they must rot. I figure that some of the ones I was looking at must have started out innocently, as they seemed to have the air of a small, functional house with lots of haphazard additions built by someone’s pitied brother in law the contractor. So, what is it that dooms them all, regardless of origin or conception?

Yes! Blame the Name!

What they all have in common is their names. Generally, it’s common practice to give classes noun names and methods the name of noun-verb pairs (with optional modifiers). So, you have a “Customer” class with a “string GetFullName()” method. Service oriented classes tend to have noun names that are or contain deverbal nouns, such as CustomerValidityEvaluator or EventAggregator…. or, CustomerHelper. We’ll get back to this in a moment.

Humans are habitual organizers, and programmers tend to be moreso. We like to categorize, arrange, place in buckets, and otherwise assign hierarchy. But, we’re also naturally lazy. If we have a framework in place for organization, we’re happy enough to behave incrementally. You get and keep this:

If, on the other hand, no framework exists, the Broken Windows Theory goes into effect, and you get and keep this:

Now, whether people pay lip service to Big Up Front Design (BUFD) or not, most people actually practice some kind of emergent design in their code or another. You don’t necessarily know exactly what every class is going to do when you start, so some exploratory coding is probably done on the side, even in the most Waterfall-committed shop. At some point, each class starts with a (relatively) blank slate, and there is some uncertainty as to what this class is going to do, exactly. TDD lets you drive this with correct functionality as a client. BUFD dictates to you everything that it should do, but you still deviate because BUFD is about as practical as an ice cream grill. No process at all renders the slate totally blank.

As the coding phase wears on, more functionality will be added to this class, and it might stay neat and organized, or it might start to look increasingly like the second image. And, I submit that a big determining factor in whether or not that happens is the name of the class. A class with a name like “CustomerNameTruncator” is likely to repel random functionality like a rain-exed windshield because people are hesitant to do willfully wrong things in code. If you’re weighing your options as to where to place a method that reads a customer table in the database, you’re probably going to avoid CustomerNameTruncator because if anyone sees your name on that, they’ll think that you’re daft.

Well, you don’t want that, so you get clever. You ‘refactor’ the “CustomerNameTruncator” to be called “CustomerHelper” and throw your database method in there. Problem solved! Nobody can tell you that it doesn’t belong there because your method is certainly helpful. And now, we have a design pattern. Anything that might be helpful to customers goes in here. The class can truncate the customer’s name, and it can read the customer table, and, even send emails to customers informing them about new products that they might enjoy. I mean, why not? That’s helpful, and with the database method in there, it already knows their email addresses. Heck, it might be even more helpful if it read the new offerings from the offerings table, too. This class just keeps getting more and more helpful.

And, as it gets more and more helpful, its maintainability index plummets, and it rots. It does everything under the sun, and it gets coupled to more and more unrelated, unfocused things. The person that changes its name (or names it that from the get-go) has broken the first window in the building, and every other developer that stops by thinks it might be sinfully satisfying to toss a rock through another window.

That is the problem with this naming scheme. It’s so vague that whatever functionality you add to it pretty much always belongs there, at least according to its name. Good OOP does not allow this practice in your design, but the name begs to differ. It pours you a stiff drink and invites you to throw your dirty clothes on the floor and worry about it later.

As an exercise, if you see a class named in this fashion, open it up and try to describe its behavior in a simple sentence. I don’t even need to open “CustomerNameTruncator” to do that. But, how do I describe “CustomerHelper”? I bet it’s a run-on sentence with a lot of “ands”, and probably a number of “ifs” and “buts” as well, for good measure. When you’re describing a class’s function and you’re saying “and” a lot, there’s a good chance that class has too many responsibilities.

Well, The Windows are Already Broken, so Whatcha Gonna Do?

The fact that you’ve got Helpers and Managers liberally sprinkled throughout your code base is not an unsolvable problem. In the formulation of the Broken Window Theory, the important realization was that it was time to fix all of the windows and keep them fixed. Same thing applies here.

The very first step is to change the name of those classes. Don’t put it off because you don’t have time for a refactoring or be intimidated. Just change the name. If it has a bunch of responsibilities, pick one. Turn “CustomerHelper” back into “CustomerNameTruncator” and let the database and email methods look ridiculous. Your path to refactoring will be obvious.

Also, realize that it always looked ridiculous to the eye of a critical OO designer. Naming it helper won’t fool anyone who actually looks at the class, or any objective calculator of maintainability. The fact that you can argue that the class’s methods don’t disagree with its name doesn’t alter another fact — the class is a hulking, 4000-line behemoth with no clear single purpose. You don’t get points for coming up with a name so vague as to be meaningless, anymore than someone would for putting all code in a giant method called “DoWhatMustBeDone()”.

And then, take the experience with you and pass it on. It isn’t just “Helpers” and “Managers” that are the problem – it’s any class or method with a name vague enough that it could do anything imaginable and still be considered logically consistent. Keep an eye out for that in your code or in that of others and steer empty rooms towards being well organized as they fill up, rather than messy.