SomethingManager Classes

Posted by Jeremy Miller on March 30, 2006

[EDIT] – The key point I was trying to make is to watch out for class names that do not communicate their intention.  If SomethingManager has a specific meaning in your system and your team is happy with it, that’s all good.  A code smell means something to be concerned about, not automatically something that is wrong and has to be changed.


 


Jeff Atwood on Coding Horror has a good post up called “I shall call it.. SomethingManager“ that’s collecting some good comments as well. 


C. Keith Ray also had a related post late last year about “SomethingUtility” classes.


For my part I’d label classes named ”SomethingManager” and “SomethingUtility” as a full fledged code smell.  I.e., an indication that something *might* be wrong with your code that requires some of your attention. 


Ideally all classes have a distinct role and responsibility within the greater system.  That role and responsibility should be reflected by the class name.  Like Keith says, a “utility” or “manager” class might be indicative of a procedural system where class responsibilities aren’t being adequately understood or modeled.  If you can’t think of a better name for a class than “SomethingManager” it’s a sign that you don’t fully understand the scope of its responsibility.


I work with some legacy code that has a lot of catch all utility classes that just lump a lot of broadly related methods together.  The result is a handful of very large classes that have too many unrelated responsibilities and a considerable amount of difficulty in locating code. 


The analogy I would try to draw is which situation would you rather have, “the gardening shears are on the tool shelf” or “they’re in the garage somewhere?”  Figuring out where to put a responsibility in the system in such a way that others could readily find it later is a key exercise in creating maintainable systems.


 

About Jeremy Miller

Jeremy is the Chief Software Architect at Dovetail Software, the coolest ISV in Austin. Jeremy began his IT career writing "Shadow IT" applications to automate his engineering documentation, then wandered into software development because it looked like more fun. Jeremy is the author of the open source StructureMap tool for Dependency Injection with .Net, StoryTeller for supercharged acceptance testing in .Net, and one of the principal developers behind FubuMVC. Jeremy's thoughts on all things software can be found at The Shade Tree Developer at http://codebetter.com/jeremymiller.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.backlinkschecker.ws/backlinks/buylinks.html Purchase High PR Backlink

    I agree with the above comment , thanks

  • jmiller

    Well put Robert.

    “You probably should of decreed that there is definately a place for the Manager class. ”
    — that’s why you always leave comments turned on;)

    I think you might be reading these posts a little bit too literally, or more likely my post is too vague. I agree that there is a place for use case controllers like your “Manager” classes, but I’m still iffy about “Manager” as the *name* because “manage” is so overloaded.

    I’ve edited the post to put “might be” in the appropriate places.

  • Robert

    Jeremy, I’m all about everything having it’s place. But it’s your support of Keith’s comment that automatically says it’s indicative of being wrong and/or being misused.

    I took your reference of Keith statement as you being supportive of that statement:
    Like Keith says, a “utility” or “manager” class is indicative of a procedural system where class responsibilities aren’t being adequately understood or modeled. If you can’t think of a better name for a class than “SomethingManager” it’s a sign that you don’t fully understand the scope of its responsibility.

    Where the [a "utility" or "manager" class is indicative of a procedural system where class responsibilities aren't being adequately understood or modeled] statement says, I’m probably wrong in defining behavior in a Manager class. Like Eric said, it’s just not that cut and dry. You probably should of decreed that there is definately a place for the Manager class.

    I agree, that if classes are not defined well, or implement behavior strangely they should be reconsidered, but this doesn’t fall just within the SomethingManager. It falls with any class a developer builds.

  • jmiller

    Eric,

    If it was easy they wouldn’t have to pay us very much

  • http://www.codebetter.com/blogs/eric.wise ewise

    Thanks for the reply Jeremy. I think we share a similar opinion on this, I just didn’t read it like that in the original post.

    One thing I’ve seen as an alternative to lazy loading (one problem with lazy loading is if you’re in a routine with 5 child items/collections you end up with 5 database calls) is the LoadDeep() functionality.

    CustomerManger.LoadDeep(customerId, bool loadPhoneNumbers, bool loadAddresses, bool loadInvoices, etc)

    The routine then batches into one sql call and uses the datareader.nextresult() to keep loading things up.

    The only flaw I saw with the implementation was that if you added a new child to be loaded you broke the signature. I hate breaking signatures.

  • jmiller

    Robert,

    One more quick thing, if your team is familiar with the use case controller type pattern and know you’re using it on a system, then yes I agree, “SomethingManager” is description.

  • jmiller

    Robert,

    I’m sorry you feel that way. If you look I did say a code smell means something *might* need more thought, not “it’s automatically wrong.” What I was writing about wasn’t the Manager/Service pattern, but classes that don’t have a well-defined role or meaning.

    I’m certainly not suggesting adding more complexity here. You said something key though about a Manager class being a logical entry point and a coordinator and I’m cool with that. I’ve more often seen a SomethingManager class being a grab bag of unrelated functions. That’s harmful.

    “Now I have to name my Something class, SomethingToDoWithNothing, and now anyone wanting to look at this will then have to dig around in the code and/or Unit Tests to find out what’s going on! ”

    Robert, I was *trying* to say use good naming that describes the function of a class. I agree that SomethingToDoWithNothing is probably a really bad name.

  • jmiller

    Eric,

    Thanks for the comment.

    If your team reads “Manager” as “Persistance,” that’s fine, but don’t you think “Persistor” or “Mapper” might be more descriptive?

    And I think Customer customer = new Customer(id) is, well not evil, but not real good either. I strongly prefer:

    CustomerMapper mapper = new CustomerMapper();
    Customer customer = mapper.Find(id);

    with a

    Customer customer = mapper.FindPartial(id);

    if you so desire and the ability to do

    Customer customer = new Customer();
    // set properties

    inside a unit test.

    I might disagree with splitting the business logic between Customer and CustomerManager though. If there is any validation logic around whether a customer has to have an address I would probably want that centered in the Customer, even if it’s really in a child member. You can always beat the performance problem of loading the entire object graph with lazy loading and virtual proxies that are be built into any mature O/R tool.

    I’m definitely in the camp that says you don’t mix any persistence functionality in the business domain classes, so I’m all on board with your CustomerManager scheme. I really don’t like internal data mapping inside the business classes. I think I promised David Hayden I’d post on that someday.

    We’re happily using NHibernate now for all new code, but I don’t have the slightest problem with going around NHibernate to do bulk updates, inserts, or deletes when that’s easier.

  • Robert

    The more I read this blog, the more I see the posts leaning towards being a code hater. It’s like being an elitist developer where everything has to be done his/her way in order for it to be correct.

    http://patternshare.org/default.aspx/Home.MF.ServiceLayer
    Let’s see, the Manager/Service pattern is well documented by many.
    A Service Layer defines an application’s boundary [Cockburn PloP] and its set of available operations from the perspective of interfacing client layers. It encapsulates the application’s business logic, controlling transactions and coordinating responses in the implementation of its operations.

    So right, Patterns, in general trigger an immediate understanding of what the class represents. The SomethingManager, tells me, a developer that has never set foot in this application, ok, this is my entry point into Something. I’m able to use something and find it’s behavior through this SomethingManager API.

    So all of a sudden, we’re all wrong! Damn it, I’ve got to stop and start using some more complicated schemes. Now I have to name my Something class, SomethingToDoWithNothing, and now anyone wanting to look at this will then have to dig around in the code and/or Unit Tests to find out what’s going on!

    Wait, this sounds awefully familiar to this post about always wanting to re-think the simplicity, and starting to over-engineer.
    http://benjismith.net/index.php/2005/09/30/hate-frameworks/

  • http://www.codebetter.com/blogs/eric.wise ewise

    Interesting… while I agree that poorly named classes are a code smell, I often see (and have used) the “manager” concept to group business logic that you don’t want to embed in business objects themselves.

    For example, a customer class. You could put things like .AddAddress() on the customer class but then you have to actually have the customer class available and additionaly you’re polluting the customer business object with address related stuff.

    So what I see happening in many cases is a class called CustomerManager which has some static methods like:

    AddCustomerAddress(customerId, Address)
    RemoveCustomerAddress(customerId, addressId)
    SaveCustomer(Customer)
    LoadCustomer(Id)
    LoadCustomerAddresses(Id)

    It seems to cut down on instances where you need to have the whole object instantiated and you may just want to use a key. Otherwise you’re doing something in code like:

    Customer customer = new Customer(id);
    customer.RemoveAddress(addressId);

    Which looks well and good, but does the new(id) method load the customer from the database? If it does, and in this case you really don’t need all that data how do you code around that? Are you adding a bunch of statics to the customer object? Is this the cleanest way?

    This isn’t a topic that I think is cut and dry. =)

  • http://www.winterdom.com/weblog Tomas Restrepo

    Jeremy:

    I would add SomethingHelper to this list as well. I do try to avoid these kind of things in my own code, but sometimes one or two static utility classes are useful. Some people do abuse them outright, though :)