In software development, indications of “bad” or poorly designed code have been given the title of “code smell(s)”. If it smells bad, it is probably bad. One of the smells I’ve learned to identify is what I’ll call “Stinky Managers.”

This smell is easily identifiable – in software code, if you see anything post-fixed with “Manager” – it probably stinks.

Why So Stinky?

The main reason “Managers” end up becoming so smelly is that they do someone else’s job. Imagine that you were a warehouse stocker for a super market chain. Your basic job would be:

  1. You are told what items to package.
  2. You package them.
  3. You are told when the delivery truck is coming to pick up the packages you’ve made.
  4. When the truck arrives, you (or someone with a forklift) puts the packages into the truck.

What often happens in software where “Managers” are used is this (to mimic how the software code would read).

  1. You are told what items to package.
  2. Your manager barges in and packages them.
  3. You are told when the delivery truck is coming to pick up the packages (well… sometimes it may be that your manager knows when the truck will arrive and you don’t).
  4. When the truck arrives, your manager puts the packages into the truck.

What’s going on? You should be packing and stocking, not your manager. It’s an Inversion of Control. You should be in control of packing, not your manager.

Here’s a sample (using pseudo-code) to illustrate:

WareHouseManager.PackItems(stocker, itemsToPack);
WareHouseManager.PackTruck(stocker, deliveryTruck);

All the stocker is doing in this example is being a placeholder for some data (the packaged food items). He doesn’t do anything. In Object-Oriented programming, objects exist to do something, not to merely hold data. This phenomenon is known as anemic objects / classes. Objects that merely hold data are anemic because they are pale and sick – sick people can’t do anything. Other people have to take care of them and do what otherwise should be the sick person’s responsibility. Like-wise, anemic objects appear when they can’t take care of themselves and other objects are doing their work for them.

Long-Term Managers Become Bloated

What if we had a new process: confirm the packages were successfully shipped?

WareHouseManager.ConfirmDeliverySuccessful(stocker); 

//Which may also look like this:

WareHouseManager.ConfirmDeliverySuccessful(stocker.getDeliveryStatus());

Whether the delivery status is asked for explicitly as a parameter or inside the function – the stocker must expose his internal data since the manager (in this code) is responsible for confirming the delivery.

Just because we are passing objects around doesn’t mean this is Object Oriented Programming. Whether on the inside or outside of ConfirmDeliverySuccessful(), the Manager is directly asking for / accessing data that belongs to the stocker.

Once you start using a manager you naturally start deferring all actions in your system to him. That’s the opposite of good software design – every object should have one responsibility (or one cohesive purpose) and should hide his own data.

Imagine a system where there are hundreds of actions (a common sight for professional developers). These manager objects end-up becoming huge God-like objects (i.e. they know everything and can do anything) bloated with hundreds to thousands of lines of code. Yes, those are code smells.

How To Smell Better

The main issue here is that the manager is doing someone else’s job. The clear fix then is move the action / behavior from the manager to the subject of the action. Let’s put that on it’s own line to make it sink in:

Move the action / behavior from the manager to the subject of the action.

Look at the code below and ask yourself: who is the manager performing the on?

WareHouseManager.PackItems(stocker, itemsToPack);

Instead of the manager packaging the products, we move the PackItems() action from the WareHouseManager to the Stocker – who is the subject of the interaction. In the case of these know-it-all managers this is pretty much the remedy to the sickness.

Here’s what that new code might look like:

stocker.PackItems(itemsToPack);
stocker.PackTruck(deliveryTruck);
stocker.ConfirmDeliverySuccessful();

The code is more readable, shorter and in the long run more maintainable. The stocker knows about his own packages and can deal with it himself like an adult. And you have less objects to worry about. If you need a new action / behavior don’t put it in a know-it-all manager. Think about who should be doing the action. Does the code read like real-life / make (common) sense?

Bonus

Using the chainable method pattern you could also reduce the code further (which was not possible before):

stocker.PackItems(itemsToPack)
       .PackTruck(deliveryTruck);
       .ConfirmDeliverySuccessful();