Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
5  
Unit testing tools that I have seen tend to have mechanisms for accessing those private methods; at least in .NET, not sure about java. This is a good pattern, it's adherence to single responsibility principle and increases maintainability and readability for sure. –  Jimmy Hoffa May 17 '13 at 21:21
1  
As far as I'm aware Java doesn't have any mechanisms for accessing private methods. Reflection is one rather ugly work around but I would prefer to use something cleaner. –  user86834 May 17 '13 at 21:24
1  
The .NET unit test tools use reflection for this I believe, but it's not ugly because the unit testing framework creates the proxy object for you and handles all of that. I'd be surprised if java doesn't have unit test frameworks that create similar proxies so the reflection hackery is transparent; and since it's only done in a unit test, there's no concern regarding reflection as bad practice –  Jimmy Hoffa May 17 '13 at 21:27
add comment

marked as duplicate by gnat, BЈовић, Kilian Foth, Jalayn, Glenn Nelson Jun 4 '13 at 12:30

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

5 Answers

up vote 24 down vote accepted

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.

share|improve this answer
    
Thanks for the link, interesting. –  user86834 May 17 '13 at 21:40
    
"You should only be testing the public interface" - this is repeated again and again by "true TDD believers". I have a different opinion. When you build complex classes from simpler classes, the simpler classes have a public interface and should be unit tested. When you build complex functions from simpler functions, it is the often same situation - often its is still worth unit testing the simpler ones, (testing only the public functions is not a unit test any more, its an integration test). –  Doc Brown May 17 '13 at 22:19
    
@DocBrown If you're practicing TDD, you've near 100% code coverage on all your private functions. You also don't particularly care how they behave internally, so long as they satisfy their use through the public functions 'API' — what's the benefit to be gained by testing them? –  anotherdave May 17 '13 at 22:25
1  
@DocBrown: I think you'll find it's more a BDD mindset than a TDD mindset. But, again, if I'm running into the problem you're talking about, I'll move some of the complexity into another class. –  pdr May 17 '13 at 23:37
1  
@DocBrown If those internal details can be generalized in order to be used in multiple components, then you extract them to a separate component, complete with its own unit tests. Again, you never directly test private methods. –  Andres F. Jun 3 '13 at 17:20
show 5 more comments

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).

share|improve this answer
    
Really? It I had a string that needed to be parsed, lets say your Foo class is the parser containing the parser methods. If I was to move them out of the Foo class into a class with other methods that would be violating the Single Responsibility Principle. –  user86834 May 18 '13 at 8:37
    
@user86834 Obviously the terms are abstract when we're talking in Foos and Bars, but my point is that if you're taking a Bar object into your Foo class & then modifying its internals excessively using getters() and setters(), you'd be better off to put methods on Bar directly. If Foo is doing 10 things to the internals of Bar, it sounds like it knows too much about it — it should tell Bar to give information about itself, not ask it questions & form its own answer. If you then break SRP, you can separate your Parser functions into a class as a private field of Bar. –  anotherdave May 18 '13 at 16:28
1  
@user86834 Your example is different than how the code reads. Parsing a string, then passing the parsed string into another method is different than passing the same exact object (string or otherwise) into 2-5 methods. –  Aaron Kurtzhals Jun 3 '13 at 16:51
add comment

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.

share|improve this answer
    
Why the downvote? I don't see what' –  Mathew Foscarini May 18 '13 at 17:55
add comment

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

share|improve this answer
add comment

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

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.