Should you refactor all helper classes into small obects with state?

Question

Imagine you want to have a room painted. You are the owner of this room and a Painter is capable of painting the room. There are different types of Painters: a BluePainter, a RedPainter etc. For every color there exists a Painter! You don't care in what color the room is going to be painted, as long as the room gets painted. This is a strategy pattern situation.

(By the way, I know that it makes more sense to have different types of Colors and give Paint or Painter a Color instance, but let's use my example where every type of Painter is only capable of painting one color.)

There are two general ways to get my room painted. I am going to illustrate both these ways in two different situations.

In the first situation, the Painter instantiation does not require any parameters.

Option 1: you have been provided a Painter instance whom you tell $painter->paintRoom(). The Painter then goes and paints the room.

One could say that the Painter has no state and is an instance of a Helper class.

Option 2: you have been provided a PainterFactory instance which you tell $painter_factory->createPainter()->paintRoom(). The Painter then goes and paints the room.

In this situation the PainterFactory is the one without state and is in instance of a Helper class. We'll see why Painter will have state in the next situation.

In the second situation, the Painter instantiation requires one parameter. A Painter is instantiated by: new Painter(paint_thickness). The paint thickness determines how thick the paint layers on the walls should be.

Option1: you have been provided a Painter instance whom you tell $painter->paintRoom(paint_thickness). The Painter then goes and paints the room. Imagine that the Painter is only certified/capable to apply paint of a certain thickness.

It is more obvious now that the Painter is a Helper class.

Option 2: you have been provided a PainterFactory instance which you tell $painter_factory->createPainter(paint_depth)->paintRoom(). The Painter then goes and paints the room.

In this situation the PainterFactory remains to be the Helper class. The Painter will have proved to have state: it contains a paint_thickness property. The Painter is not a Helper class for this option.

The reason I think Painter is a Helper class in the first situation, and PainterFactory is a Helper class in the second situation is that it will never be necessary to have more than one instance of a Painter class.

To summarize: it is often possible to either use one instance of a class for certain functionality or to rewrite the class to actually contain state. The first option represents the use of Helper classes. Helper classes are often criticized as being procedural and static in nature and thus bad in OOP and a potential technical debt.

However, instantiation of objects which do have state must either happen through a Helper factory (as we saw in the example) or by direct instantiation. Since the Helper factory is still a Helper, it seems that the use of Helper classes is still not completely avoided - and direct instantiation is essentially static in nature as well.

So my question is, are Helper classes really bad? Should one favor an object with state over an object without state but with behavior? But then again, what about small objects that don't really have behavior other than the behavior that exists in their one method (e.g. imagine rewriting a large Helper class called Math into separate classes such as Multiplication, Addition, Division etc.).

What are some good general rules to choose an option in a Painter situation?

• gnat: Possible duplicate of Which is a better practice - helper methods as instance or static?
• GHP: Adding mutable state where its not needed is a greater sin than static Helper classes, in my opinion, which perhaps indicate poor function organization at worst, which is much preferable to actual software bugs.
• Rogério: @Graham The state in Painter is clearly not mutable. And even if it was, it would most likely be perfectly fine since each Painter object would get instantiated and used by a single thread. Mutable state is only (potentially) a problem when it's concurrently accessed by multiple threads. So, in this situation I see no "sin" at all in having a stateful Painter object instead of a stateless singleton class (which the OP calls a "Helper" class).

Answer 1

I would choose the variant where paint_thickness is a parameter of the paintRoom method. It can be optional:

$some_painter->paintRoom(paint_thickness);
$painter_factory->createPainter()->paintRoom(paint_thickness);

If there is no reason for Painter to remember the thickness, then lets not store it inside him. Putting it inside would just make things unnecessarily more statefull.

Also just because Painter doesn't hold any state, I wouldn't call it a helper class. The feared helper classes are the ones that are used just as a namespace to hold several utility static methods.

• user2180613: The paint_thickness is only an illustration. Imagine that the Painter is only certified/capable to apply paint of a certain thickness or something like that. I needed the Painter to hold a property in order to illustrate the difference between the two situations. The reason I think Painter is a helper class in the first situation, and PainterFactory in the second situation is that it will never be necessary to have more than one instance of a Painter class.
• michalsrb: Ok, if it makes sense for the property to be stored in the object, then of course do it. You are right that when the Painter class doesn't hold any data, you would never need more than one instance of it (and the factory could even always give you this one instance), but in my opinion it is still not a helper class. It still has single responsibility (painting), it has (or could have) an interface that it could share with other painters and be exchangeable with them. No problem here.

Answer 2

First thing I would point out there is HUGE difference between immutable state and mutable state. It is mutable state that is something to be careful about. Immutable state is generally not much different than no state. So in your case, painter keeping it's paint_thickness parameter is not an issue, as long as that value is set during construction and then kept read-only.

Second thing to note is that your options might make sense when painter is created and used in same place. In that case, there is not much difference between your options. But it becomes a big difference when place where painter is created and place where it is used are separate. Then, it becomes question of what parameters are available at what place. If paint_thickness is known during painter's creation, pass it there. If it is known during usage, pass it there. This is the whole point of abstraction. If caller of paint method doesn't care about paint_thickness, it shouldn't be forced to know it so it can pass it into the method. Maybe paint thickness depends on color or type of surface and I as a customer shouldn't have any say about it. This is result of simple rule : object's (or interface's) API is determined by the one who uses it, not by what it provides.

Answer 3

Stateless classes are not "bad", you can still mock them out if that is what you need for testing purposes. The kind of "helper classes" (which is not a term with a clear and widely accepted definition) which might be problematic are static classes, because you cannot easily replace them by a mock when you want to unit test other code which uses those classes.

So the anwer is clearly "no, you should not refactor all stateless classes into objects with state". You might consider to replace static classes by non-static ones, but only if you see the possible necessity to unit test other code which might depend on those classes.