I can't unit test my class without exposing private fields -- is there something wrong with my design?

Question

I have written some code which I thought was quite well-designed, but then I started writing unit tests for it and stopped being so sure.

It turned out that in order to write some reasonable unit tests, I need to change some of my variables access modifiers from private to default, i.e. expose them (only within a package, but still...).

Here is some rough overview of my code in question. There is supposed to be some sort of address validation framework, that enables address validation by different means, e.g. validate them by some external webservice or by data in DB, or by any other source. So I have a notion of Module, which is just this: a separate way to validate addresses. I have an interface:

interface Module {

  public void init(InitParams params);

  public ValidationResponse validate(Address address);
}

There is some sort of factory, that based on a request or session state chooses a proper module:

class ModuleFactory {

  Module selectModule(HttpRequest request) {
       Module module = chooseModule(request);// analyze request and choose a module
       module.init(createInitParams(request)); // init module
       return module;
  }

}

And then, I have written a Module that uses some external webservice for validation, and implemented it like that:

WebServiceModule {
   private WebServiceFacade webservice;     

   public void init(InitParams params) {
      webservice = new WebServiceFacade(createParamsForFacade(params));
   }

   public ValidationResponse validate(Address address) {
      WebService wsResponse = webservice.validate(address);
      ValidationResponse reponse = proccessWsResponse(wsResponse);
      return response;
   }

}

So basically I have this WebServiceFacade which is a wrapper over external web service, and my module calls this facade, processes its response and returns some framework-standard response.

I want to test if WebServiceModule processes reponses from external web service correctly. Obviously, I can't call real web service in unit tests, so I'm mocking it. But then again, in order for the module to use my mocked web service, the field webservice must be accessible from the outside. It breaks my design and I wonder if there is anything I could do about it. Obviously, the facade cannot be passed in init parameters, because ModuleFactory does not and should not know that it is needed.

I have read that dependency injection might be the answer to such problems, but I can't see how? I have not used any DI frameworks before, like Guice, so I don't know if it could be easily used in this situation. But maybe it could?

Or maybe I should just change my design?

Or screw it and make this unfortunate field package private (but leaving a sad comment like // default visibility to allow testing (oh well...) doesn't feel right)?

Bah! While I was writing this, it occurred to me, that I could create a WebServiceProcessor which takes a WebServiceFacade as a constructor argument and then test just the WebServiceProcessor. This would be one of the solutions to my problem. What do you think about it? I have one problem with that, because then my WebServiceModule would be sort of useless, just delegating all its work to another components, I would say: one layer of abstraction too far.

Answer 1

Yes, your design is wrong. You should do dependency injection instead of new ... inside your class (which is also called "hardcoded dependency"). Inability to easily write a test is a perfect indicator of a wrong design (read about "Listen to your tests" paradigm in Growing Object-Oriented Software Guided by Tests).

BTW, using reflection or dependency breaking framework like PowerMock is a very bad practice in this case and should be your last resort.

Answer 2

I agree with what yegor256 said and would like to suggest that the reason why you ended up in this situation is that you have assigned multiple responsibilities to your modules: creation and validation. This goes against the Single responsibility principle and effectively limits your ability to test creation separately from validation.

Consider constraining the responsibility of your "modules" to creation alone. When they only have this responsibility, the naming can be improved as well:

interface ValidatorFactory {
  public Validator createValidator(InitParams params);
}

The validation interface becomes separate:

interface Validator {
  public ValidationResponse validate(Address address); 
}

You can then start by implementing the factory:

class WebServiceValidatorFactory implements ValidatorFactory {
  public Validator createValidator(InitParams params) {
    return new WebServiceValidator(new ProdWebServiceFacade(createParamsForFacade(params)));
  }
}

This factory code becomes hard to unit-test, since it is explicitly referencing prod code, so keep this impl very concise. Put any logic (like createParamsForFacade) on the side, so that you can test it separately.

The web service validator itself only gets the responsibility of validation, and takes in the façade as a dependency, following the Inversion of Control (IoC) principle:

class WebServiceValidator implements Validator {
  private final WebServiceFacade facade;

  public WebServiceValidator(WebServiceFacade facade) {
    this.facade = facade;
  }

  public ValidationResponse validate(Address address) {
    WebService wsResponse = webservice.validate(address);
    ValidationResponse reponse = proccessWsResponse(wsResponse);
    return response;
  }
}

Since WebServiceValidator is not controlling the creation of its dependencies anymore, testing becomes a breeze:

@Test
public void aTest() {
   WebServiceValidator validator = new WebServiceValidator(new MockWebServiceFacade());
   ...
}

This way you have effectively inverted the control of the creation of the dependencies: Inversion of Control (IoC)!

Oh, and by the way, write your tests first. This way you will naturally gravitate towards a testable solution, which is usually also the best design. I think that this is due to the fact that testing requires modularity, and modularity is coincidentally the hallmark of good design.