29

Let's say I have a procedure that does stuff:

void doStuff(initalParams) {
    ...
}

Now I discover that "doing stuff" is quite a compex operation. The procedure becomes large, I split it up into multiple smaller procedures and soon I realize that having some kind of state would be useful while doing stuff, so that I need to pass less parameters between the small procedures. So, I factor it out into its own class:

class StuffDoer {
    private someInternalState;

    public Start(initalParams) {
        ...
    }

    // some private helper procedures here
    ...
}

And then I call it like this:

new StuffDoer().Start(initialParams);

or like this:

new StuffDoer(initialParams).Start();

And this is what feels wrong. When using the .NET or Java API, I always never call new SomeApiClass().Start(...);, which makes me suspect that I'm doing it wrong. Sure, I could make StuffDoer's constructor private and add a static helper method:

public static DoStuff(initalParams) {
    new StuffDoer().Start(initialParams);
}

But then I'd have a class whose external interface consists of only one static method, which also feels weird.

Hence my question: Is there a well-established pattern for this type of classes that

  • have only one entry point and
  • have no "externally recognizable" state, i.e., instance state is only required during execution of that one entry point?
Steven Jeuris
  • 5,804
  • 1
  • 30
  • 52
Heinzi
  • 9,646
  • 3
  • 46
  • 59
  • I have been known to do things like `bool arrayContainsSomestring = new List(stringArray).Contains("somestring");` when all I cared about was that particular piece of information and the LINQ extension methods are not available. Works fine, and fits inside an `if()` condition without needing to jump through hoops. Of course, you want a garbage-collected language if you're writing code like that. – user Nov 08 '12 at 09:07
  • If it's [tag:language-agnostic], why add [tag:java] and [tag:c#] as well? :) – Steven Jeuris Nov 08 '12 at 11:01
  • I'm curious, what is the functionality of this 'one method'? It would help a great deal in determining what the best approach is in the specific scenario, but interesting question nonetheless! – Steven Jeuris Nov 08 '12 at 11:06
  • @StevenJeuris: I've stumbled upon this issue in various situations, but in the current case it's a method that imports data into the local database (loads it from a web server, does some manipulations and stores it in the database). – Heinzi Nov 08 '12 at 11:28
  • 2
    If you always call the method when you create the instance, why not make it an initialization logic inside the constructor? – NoChance Nov 08 '12 at 12:53
  • @EmmadKareem Haha! Now that you put it that way. There would be no real difference, and it clearly outlines the problem with this approach. – Steven Jeuris Nov 08 '12 at 15:02
  • @StevenJeuris: Regarding the tags: Although the question itself is language-agnostic, (a) I had the feeling that the problem is more relevant to Java/.NET developers than, for example, Perl or PHP users, (b) the examples are written in pseudo-Java/C# and (c) I was particularly interested in the opinion of Java/.NET developers (since that's what I'm using), so I wanted to attract that crowd. – Heinzi Nov 08 '12 at 22:00
  • If "instance state is only required during execution of that one entry point", then you have yourself a static method. There's no reason to construct the class. – Marco Nov 14 '12 at 07:10

7 Answers7

33

There's a pattern called Method Object where you factor out a single, large method with a lot of temporary variables / arguments into a separate class. You do this rather than just extracting parts of the method out into separate methods because they need access to the local state (the parameters and temporary variables) and you can't share the local state using instance variables because they would be local to that method (and the methods extracted from it) only and would go unused by the rest of the object.

So instead, the method becomes its own class, the parameters and temporary variables become instance variables of this new class, and then the method gets broken up into smaller methods of the new class. The resulting class usually has just a single public instance method that performs the task the class encapsulates.

anon
  • 346
  • 2
  • 3
  • 1
    This sounds exactly like what I'm doing. Thanks, at least I have a name for it now. :-) – Heinzi Nov 08 '12 at 11:30
  • 2
    Very relevant link! It does smell like an anti-pattern to me. If your method is that long, perhaps parts of it could be extracted in _reuseable_ classes or generally be refactored in a better way? I also haven't heard about this pattern before, neither can I find many other resources which makes me believe it's generally not used that much. – Steven Jeuris Nov 08 '12 at 15:05
  • Poltergeists [seems to be an appropriate anti-pattern](http://programmers.stackexchange.com/a/175120/15464). Additionally, [it doesn't seem to be a pattern, but rather a refactoring method](http://sourcemaking.com/refactoring/replace-method-with-method-object). – Steven Jeuris Nov 08 '12 at 15:58
  • 1
    @StevenJeuris I don't think it's an anti-pattern. An anti-pattern would be to clutter up refactored methods with lot parameters instead of storing this state that is common between these methods in an instance variable. – Alfredo Osorio Nov 08 '12 at 17:36
  • @AlfredoOsorio: Well, it does clutter up refactored methods with lot of parameters. They live in the object, so it's better than passing all of them around, but it's worse than refactoring to reusable components that only need a limited subset of the parameters each. – Jan Hudec Nov 09 '12 at 06:50
13

I would say that the class in question (using a single entry point) is well designed. It's easy to use and extend. Quite SOLID.

Same thing for no "externally recognizable" state. It's good encapsulation.

I don't see any problem with code like:

var doer = new StuffDoer(initialParams);
var result = doer.Calculate(extraParams);
jgauffin
  • 4,512
  • 21
  • 33
  • 1
    And of course, if you don't need to use the same `doer` again, that reduces to `var result = new StuffDoer(initialParams).Calculate(extraParams);`. – user Nov 08 '12 at 09:08
  • Calculate should be renamed to doStuff! – shabunc Nov 08 '12 at 09:39
  • @shabunc: Why does it matter for the question? – jgauffin Nov 08 '12 at 10:01
  • @jgauffin - consistency of course. To be more serious, you should not take any comment so serious. Hadn't intention to bother you, though. – shabunc Nov 08 '12 at 10:05
  • 1
    I would add that if you don't want to initialize (new) your class where you use it (probably you want to use a Factory) you have the option of creating the object somewhere else in you business logic and than pass the parameters directly to the single public method you have. The other option is to use a Factory and have on it a make method which gets the parameters and creates you object with all the params through it's constructor. Than you call your public method without params from wherever you want. – Patkos Csaba Nov 08 '12 at 10:30
  • 2
    This answer is overly simplistic. Is a `StringComparer` class which you use as `new StringComparer().Compare( "bleh", "blah" )` well designed? What about creating state when there is entirely no need to? Okay the behavior is well encapsulated (well, at least to the user of the class, [more on that in my answer](http://programmers.stackexchange.com/a/175092/15464)), but that doesn't make it 'good design' by default. As to your 'doer-result' example. This would only make sense if you would ever use `doer` separate from `result`. – Steven Jeuris Nov 08 '12 at 12:15
  • @StevenJeuris: I edited the answer a bit. But I do not agree with you. We code OOP and should try to keep classes simple. I rather use ten small well defined classes instead of one larger which is harder to understand. It do ofcourse vary from case to case, but that should be the general appraoch imho – jgauffin Nov 08 '12 at 12:17
  • @jgauffin Possibly [the main difference lies in how far to stretch the Single Responsibility Principle](http://stackoverflow.com/q/2455705/590790). _"If you take it to that extreme and build classes that have one reason to exist, you may end up with only one method per class. This would cause a large sprawl of classes for even the most simple of processes, causing the system to be difficult to understand and difficult to change."_ – Steven Jeuris Nov 08 '12 at 12:25
  • Another relevant reference relating to this topic: "Divide your programs into methods that perform one identifiable task. Keep all of the operations in a method at **the same level of abstraction**." - [Kent Beck](http://www.amazon.com/Smalltalk-Best-Practice-Patterns-Kent/dp/013476904X) This level of abstraction is entirely up to the context for which you are designing. – Steven Jeuris Nov 08 '12 at 12:29
  • 1
    It's not one reason to exist, it's `one reason to change`. The differnce might seem subtle. You have to understand what that means. It will never create one method classes – jgauffin Nov 08 '12 at 13:03
  • @jgauffin That _was_ what that quote was saying no? Understanding that difference is what leads me to be suspicious about the OPs design. – Steven Jeuris Nov 08 '12 at 13:26
  • P.s. regarding your update. _"I would say that the class in question"_ ... you are still drawing conclusions about a design, without even knowing the context. The class in question is `Foo` with a method `Bar()`. It doesn't say anything, it simply represents a design. I [recommend you read through the entire answer which I quoted](http://stackoverflow.com/a/2463221/590790). – Steven Jeuris Nov 08 '12 at 13:41
  • Well. I recommend that you start reading the OPs question better first. Your comment saying `doesn't manage any state` is incorrect. Regarding `reason to exist`: The quote is still wrong. – jgauffin Nov 08 '12 at 13:57
  • @jgauffin When I say 'doesn't manage any state', I mean to imply the state is only created within class scope, in order to split one procedure into smaller functions. There is _no need_ for state. The state is only relevant within the call of the method. As to the quotation, we are saying the same thing. Let me emphasize it for you: _"**If you take it to that extreme** and build classes that have one reason to exist, ..."_. – Steven Jeuris Nov 08 '12 at 14:25
  • *State*: Good encapsulation promotes internal state. Saying that classes that only have internal state shouldn't exist is so wrong. *SRP*: That quote has nothing to do with SRP, since SRP says nothing about ***have one reason to exist***. – jgauffin Nov 08 '12 at 14:32
  • @jgauffin Neither am I saying anything about 'classes that only have internal state shouldn't exist'. You are pulling several [straw man arguments](http://en.wikipedia.org/wiki/Straw_man) on me and clearly don't understand my argumentation, so I'm not continuing this discussion until you can point out what it is you don't understand about it, or which part of the argumentation you disagree with. Direct quoting (within context) would be preferred. – Steven Jeuris Nov 08 '12 at 14:37
  • As to [reply _directly_ to your last remark (again)](http://programmers.stackexchange.com/questions/175070/pattern-for-a-class-that-does-only-one-thing#comment333970_175071), if you would have read the entire thing I was referencing (since you seem to clearly misinterpret the 'if you take it to that extreme' part), here is another quote from _the same text_: _"The reason that a class should have one reason to change, instead of one reason to exist ..."_. – Steven Jeuris Nov 08 '12 at 14:46
8

From a SOLID principles perspective jgauffin's answer makes sense. However, you shouldn't forget about the general design principles e.g. information hiding.

I see several problems with the given approach:

  • As you pointed out yourself people don't expect to use the 'new' keyword, when the object created doesn't manage any state. Your design reflects it's intention. People using your class might get confused as to what state it manages, and whether subsequent calls to the method might result in different behavior.
  • From the perspective of the person using the class the inner state is well hidden, but when wanting to make modifications to the class or simply understanding it, you are making things more complex. I have already written a lot about the problems I see with splitting methods just to make them smaller, especially when moving state to the class scope. You are modifying the way your API should be used just in order to have smaller functions! That in my opinion is definitely taking it too far.

Some related references

Possibly a main point of argument lies in how far to stretch the Single Responsibility Principle. "If you take it to that extreme and build classes that have one reason to exist, you may end up with only one method per class. This would cause a large sprawl of classes for even the most simple of processes, causing the system to be difficult to understand and difficult to change."

Another relevant reference relating to this topic: "Divide your programs into methods that perform one identifiable task. Keep all of the operations in a method at the same level of abstraction." - Kent Beck Key here is "the same level of abstraction". That doesn't mean "one thing", as it is often interpreted. This level of abstraction is entirely up to the context for which you are designing.

So what is the proper approach?

Without knowing your concrete use case it is hard to tell. There is a scenario in which I sometimes (not often) use a similar approach. When I want to process a dataset, without wanting to make this functionality available to the entire class scope. I wrote a blog post about it, how lambdas can even further improve encapsulation. I also started a question on the topic here on Programmers. The following is a latest example of where I used this technique.

new TupleList<Key, int>
{
    { Key.NumPad1, 1 },
            ...
    { Key.NumPad3, 16 },
    { Key.NumPad4, 17 },
}
    .ForEach( t =>
    {
        var trigger = new IC.Trigger.EventTrigger(
                        new KeyInputCondition( t.Item1, KeyInputCondition.KeyState.Down ) );
        trigger.ConditionsMet += () => AddMarker( t.Item2 );
        _inputController.AddTrigger( trigger );
    } );

Since the very 'local' code within the ForEach isn't reused anywhere else, I can simply keep it in the exact location where it is of relevance. Outlining code in such a way that code which relies on each other is strongly grouped together makes it more readable in my opinion.

Possible alternatives

  • In C# you could use extension methods instead. So operate on the argument directly you pass to this 'one thing' method.
  • See whether this function actually doesn't belong to another class.
  • Make it a static function in a static class. This most likely is the most appropriate approach, as is also reflected in the general APIs you referred to.
Steven Jeuris
  • 5,804
  • 1
  • 30
  • 52
  • I found a possible name for this anti-pattern [as outlined in another answer, Poltergeists](http://programmers.stackexchange.com/a/175120/15464). – Steven Jeuris Nov 08 '12 at 15:34
4

I would say it's pretty good, but that your API should still be a static method on a static class, since that's what the user expects. The fact that the static method uses new to create your helper object and do the work is an implementation detail that should be hidden from whoever is calling it.

Scott Whitlock
  • 21,874
  • 5
  • 60
  • 88
  • Wow! You managed to get to the gest of it a lot more concisely than I did. :) Thanks. (of course I go a bit further where we might disagree, but I agree with the general premise) – Steven Jeuris Nov 08 '12 at 14:12
2
new StuffDoer().Start(initialParams);

There is a problem with clueless developers who could use a shared instance and use it

  1. multiple times (ok if previous (maybe partial) execution does not mess up later execution)
  2. from multiple threads (not OK, you explicitly said it has internal state).

So this needs explicit documentation that it is not thread safe and if it's lightweight (creating it is fast, does not tie up external resources nor lot of memory) that it's OK to instantiate on the fly.

Hiding it in a static method helps with this, because then reusing the instance never happen.

If it has some expensive initialization, it could (it isn't always) be beneficial to prepare initializing it once and using it multiple times by creating another class that would contain state only and make it cloneable. Initialization would create state that would be stored in doer. start() would clone it and pass it to internal methods.

This would also allow for other things like persisting state of partial execution. (If it takes long and external factors, e.g. electricity supply failure, could interrupt execution.) But these additional fancy things are usually not needed, so not worth it.

Steven Jeuris
  • 5,804
  • 1
  • 30
  • 52
user470365
  • 1,229
  • 6
  • 8
2

Besides my more elaborate, personalized other answer, I feel the following observation deserves a separate answer.

There are indications that you might be following the Poltergeists anti-pattern.

Poltergeists are classes with very limited roles and effective life cycles. They often start processes for other objects. The refactored solution includes a reallocation of responsibilities to longer-lived objects that eliminate the Poltergeists.

Symptoms And Consequences

  • Redundant navigation paths.
  • Transient associations.
  • Stateless classes.
  • Temporary, short-duration objects and classes.
  • Single-operation classes that exist only to “seed” or “invoke” other classes through temporary associations.
  • Classes with “control-like” operation names such as start_process_alpha.

Refactored Solution

Ghostbusters solve Poltergeists by removing them from the class hierarchy altogether. After their removal, however, the functionality that was “provided” by the poltergeist must be replaced. This is easy with a simple adjustment to correct the architecture.

Steven Jeuris
  • 5,804
  • 1
  • 30
  • 52
0

There are several patterns can be found on Martin Fowler's P of EAA catalog;

  1. Transaction Script: Somewhat similar to the approach you mentioned.
  2. Replace method w/ Method object: Look like a good approach too as @anon mentioned.