I’ve seen my fair share of code; some written by me, some written by vast teams; and a lot in-between. And I’ve learned many of ways to spot Garbage. That is, rubbish code. Code that will bring you pain and will be the source of ‘Magic’ to the less experienced.
And I’m going to share with you a simple trick to help you spot it; meaningless class names.
Granted this is not new, Steve McConnell of Code Complete fame extols the virtue of good class names as do, no doubt, many others. But where they tell you to think hard about naming your classes, I give you the taxonomy of other people’s smells. And we all prefer to gloat than code carefully…so hear goes.
Any class with manager in it is a sure sign it was written in a hurry to fill some gap. Manager? What is it managing? Is it a collection? Does it construct objects? Dose it run salary reviews for it’s managed objects? Who knows?
If you see ‘managers’ in your code ask yourself if what they actually do could be expressed more clearly. Replace ConnectionManager with ConnectionPool, ConnectionLookupTable, OpenConnectionSource or (if you must) ConnectionFactory. Each of the later class names gives you a better idea of what the ‘Manager’ actually does.
Any class with helper in it is a sign that it was written to do some random look-up or complex object manipulation. This is sure sign that the Object hierarchy sucks in some profound way or that similar concepts are implemented differently.
Look in classes called helper and try and see why you need their help. Then change your class structure so you don’t need it.
Similar to Manager and acceptable in event handle nomenclature, but not for a class. Way too vauge.
Another meaningless ‘do-something’ word I’ve seen a few times. Typically used to name classes that should be called something like ThreadPool, MessageDispatcher or BackgroundDownloader.
Usually comes in the form of DataManipulator. Great indication that similar but not identical structures have been used to model the same thing in different places.
This one really confuses me. But I’ve seen EmailMessageObject and the like. ‘Object’ is totally redundant, we know it’s an object. I’ve not seen a class with Class in its name yet but I’m sure I’ll find one.
Every time I’ve seen a MessageArray or similar the class itself has implemented some odd data structure that was definitely not and Array. If it really is an array, you don’t need a class to represent it. And it if your ‘Array’ is doing something odd see if it fits into an existing Collection type there are many to choose from.
Incidentally the one I see most often re-implemented is dequeue which actually does not have a .Net standard library implementation, which is a pity, but LinkedList is semantically very similar.
Data Access Layers abound and having a class called DataAccessLayer is OK, I guess. Buy you only get one! Every other class should specify what data it intends to represent or operate on.
OK mini-rant over.
If you have any other rules of thumb or you think mine a themselves rubbish I’d like to hear.