Is this pattern bad? [duplicate]

Question

I notice that when I code I often use a pattern that calls a class method and that method will call a number of private functions in the same class to do the work. The private functions do one thing only. The code looks like this:

public void callThisMethod(MyObject myObject) {
    methodA(myObject);
    methodB(myObject);  // was mehtodB before
}

private void methodA(MyObject myObject) {
    //do something
}

 private void methodB(MyObject myObject) {
    //do something
}

Sometimes there are 5 or more private methods, in the past I have moved some of the private methods into another class, but there are occasions when doing so would be creating needless complexity.

The main reason I don't like this pattern is that is not very testable, I would like to be able to test all of the methods individually, sometimes I will make all of the methods public so can I can write tests for them.

Is there a better design I can use?

Answer 1

It's a good pattern. This is what Uncle Bob refers to when he talks about extracting til you drop.

It shouldn't affect your tests though. You should only be testing the public interface. Yes, you can reflect your way in and test the private methods, but you're still going to have to test them again when you test your public methods, so it doesn't solve the perceived problem.

The "problem" comes when you're testing a private method multiple times because you have multiple public methods using them.

Sometimes, in fact more frequently than you might think, this doesn't matter. Particularly if you're testing by behaviour, rather than trying to test each line of code. All you want to know is "Does this public method do what it's supposed to do?" You don't care about the implementation details.

It does matter, however, when you're testing the same piece of complex logic repeatedly, because it adds multiple tests to each behaviour. At that point, simply extract it into a service and mock it out.

Answer 2

What I would keep an eye out for though is the amount of times you're passing around your initial object parameter. If all of your private methods are operating on the main parameter, it may be a bit of a code smell. E.g. if you have:

class Foo {
    public void callThisMethod(Bar myBarObject) {
        methodA(myBarObject);
        methodB(myBarObject);
        methodC(myBarObject);
        methodD(myBarObject);
        methodE(myBarObject);

        …

        methodZ(myBarObject);

    }
}

class Bar {
    …
}

This points to low cohesion between the methods of Foo with that class — The methods seem to have more in common with Bar than with Foo. If that's the case, consider moving related methods into the class that they're operating on (see the Law of Demeter).

If that's not the case though, there's nothing wrong with having methods that do just one thing, or with testing your application via it's public methods rather than the private internals (as you can still 'exercise' all of the use cases of these methods, but it leaves you freer to refactor these methods later on if you so choose).

Answer 3

I'm wondering if you're trying to implement the Command Pattern via private methods, and you might find switching to a command pattern easier for what you are doing.

What you have now is this:

public void callThisMethod(MyObject myObject) {
    methodA(myObject);
    mehtodB(myObject)
}

private void methodA(MyObject myObject) {
    //do something
}

private void methodB(MyObject myObject) {
    //do something
}

You could implement the same thing using commands and a dispatcher.

public interface Command {
    void Execute(Context c);
}

public class Context {
    // this holds shared data
}

public class Dispatcher
{
    private List<Command> _commands = new List<Command>();

    public void Add(Command cmd) 
    {
       _commands.Add(cmd);
    }

    public void Run(Context c)
    {
       foreach(Command cmd in _commands) { cmd.Execute(c); }
    }
}

You would then implement your private methods now as commands.

public class CommandA : Command { ... }
public class CommandB : Command { ... }

Any special state information is defined in the Context class which will be passed to each command. To run the code you would add the commands to the dispatcher and run it.

Dispatcher d = new Dispatcher();
d.Add(new CommandA());
d.Add(new CommandB());
d.Run(new Context());

The advantage of this approach is that it decouples the commands from each other, and you use the Context class to make everything work together. You can also add logic to what commands are created, and what parameters are used to instantiate them.

Some other tips, you can create singletons of commonly used commands to reduce overhead in creating new ones all the time.

This approach is highly testable as each command can be isolated.

The downside is that a large collection of commands can become difficult to maintain. It's better to create fat commands that can do a lot of things, than simple small commands in large number.

Answer 4

I believe that the pattern in question is known as composed method and it is mentioned in Kent Becks Smalltalk Best practice patterns [1]. Further, he mentions that each method does one identifiable thing and that the methods called within a method should all be at the same level of abstraction.

[1] http://www.amazon.com/Smalltalk-Best-Practice-Patterns-Kent/dp/013476904X

Answer 5

For me this seems like a bad practice for two reasons.

It is unclear how those methods depend on each other. If there is an order to strictly adhere to then this order should be reflected in the code. Otherwise a hidden temporal coupling (see item G31 in Clean Code) is introduced. Making the coupling obvious can be achieved by returning an object from every method and passing that on to the next method.

If those methods don't really depend on each other an interface should be introduced with a single method. The different implementations should be kept in a collection in a field of the owning class of callThisMethod and that method should only iterate through that collection and invoke the collection elements method. This way you are adhering more to the open closed principle, because adding another functionality is just the matter of configuring another interface implementation to be kept in that list and not changing the owning class of callThisMethod