1

I read this question: How do I test a class that has private methods, fields or inner classes? and it seems that I might have a code smell, but my code is very simple to actually refactor. what is wrong in the design that I have created. I have created a delegate class for processing some actions it has three methods execute(Action); PopulateActionHandlers() and executeActionhandlers(); My class is like below:

public class DelegateHandler{
    Map<Integer,ActionHandlers> handlerMaps;

    public execute(Action action){
        populateActionHandlers(action);
        executeActionHandlers();

    }//end of execute

    //This method can create and populate more than one handlers in handlerMap
    private populateActionHandlers(action){
        handlerMap = new LinkedHashMap<ActionHandlers>();
        if (action.isMultimode()){
            handlerMap.add(1,new handler(action.getabc()));
            handlerMap.add(2,new handler(action.getabc()-1));
        }else{
            handlerMap.add(1,new handler(action));
        }

    }//end of populateActionHandlers

    //This method can execute more than one handlers in handlerMap
    private executeActionHandlers(){
        for(ActionHandler actionHandler : handlerMap.values){
            actionHandler.executeAction();
        }

    }//end of executeActionHandlers
}

Now I want to test populateActionHandlers() method with JUnit, which I made private as there is no need to expose it outside this class. If I test the execute() method then it will test both populateActionHandlers() and executeActionHandlers() methods which is testing two units at the same time, I want to test them separately. The design (I think) seems alright to me and doesnt allow any issues but then I would either change the access to the method (and only for the sake of testing it doesn't justify that in my opinion, right?) or to use reflection (is that a good idea, it does not feel right somehow, do people usually use reflection for junit testing?). So the only thing that cant be ruled out is code smell. But may be my code sinus is not really helping me So I would like to understand if I can improve this code.

Community
  • 1
  • 1
sarmahdi
  • 1,098
  • 4
  • 21
  • 61
  • 4
    I don't think testing the public method is testing two things. There's only one public operation. The class only exposes one public "thing". You'd be testing that one thing. Now, you may have multiple tests which invoke that one thing under multiple different pre-conditions in order to test individual code paths therein. But testing the public operation(s) of a class should by design also test all of its private operations. – David Apr 28 '15 at 00:23
  • Your code wont compile :) As you can see your private function populateActionHandlers() is called by public method execute(). So you either test it while testing execute(), or if you really need to test it alone then you may: a) Use reflection, which is my favourite, but condemned by some b) use mocking like powermock to change executeActionHandlers logic and still test execute() https://code.google.com/p/powermock/wiki/MockPrivate – Praeterii Apr 28 '15 at 12:01
  • @Praeterii The code was an example. I think you can call a private function from public method. Given the code, I cant test the private method directly and would have to use what you suggested, which some blogs say is code smell. Is there any problem with my design then? – sarmahdi Apr 29 '15 at 00:13

1 Answers1

0

The recommendation not to test private methods should not prevent one to do a weird design by leaving out private method, but should enforce to test only methods that have clear semantics.

Private methods are usually technical helpers. Their semantics can change if the underlying data structures change, they can even be optimized away if the calling public methods use another algorithm to achieve the same goals.

I would rewrite the programm in following way:

...
public execute(Action action){
    Map<Integer,ActionHandlers> handlerMap = populateActionHandlers(action);
    executeActionHandlers(handlerMap);
}
...

Storing results of one function into a private field only to retrieve it from this field in another function is not threadsafe and harder to maintain.

Yet this refactoring would break all (yet not writte) tests that did test the private method of your example, because the interface is changed. If you had only tested the public method, the all tests would be valid after this refactoring.

I know few cases where testing private methods is ok. While testing private methods is often avoidable, I think the checking of private state is sometimes a better alternative than only checking the public state of objects. Such checks may be not as robust (reasons as above) but the public state is often incomplete and hard to assert. In both cases I use the framework picklock which enables one to access private methods and fields in a convenient way.

CoronA
  • 7,717
  • 2
  • 26
  • 53
  • Thanks for the reply. But still does not solve the issue at hand, i need to test if the handler Map is populated correctly by populateHandlerMap() before the executeActionHandclers() is called. – sarmahdi May 08 '15 at 03:09
  • The fact that the map is populated is an implementation detail. You should not test that the map is populated but that the correct handlers are called in the execution phase. If so, the map must have been populated. If the side effect of filling the map is part of the specification of the public method, then write a getter for the map and query its contents after calling it. I would give an example (also how to call the private method), but this requires a test object that is compilable (Action* can be interfaces). – CoronA May 08 '15 at 04:12