Refactoring to test

I have a piece of code roughly equivalent to the following.

public class ConcreteThread extends OtherThread {
  private DAOfirst firstDAO;
  private DAOsecond secondDAO;
  private TransformService transformService;
  private NetworkService networkService;

  public ConcreteThread(DAOfirst first, DAOsecond second, TransformService service1, 
       NetworkService service2) {
    firstDAO = first;
    secondDAO = second;
    transformService = service1;
    networkService = service2;
  }

  public Future go() {
    Results r1 = firstDAO.getResults();
   开发者_JS百科 MyCallable c1 = new MyCallable(r1);
    return super.getThreadPool().submit(c1);
  }

  private class MyCallable implements Callable {   
    private Results result; 
    private Long count;
    private MyCallable(Results r) {
      this.result = r;
      this.count = new Long(0);
    }

    public Long call() {
      Singleton transactions = Singleton.getInstance();
      try {
        transactions.begin();
        while(result != null) {
          Transformed t = transformService.transform(r1);
          networkService.sendSomewhere(t);
          count = count += result.size();
          secondDao.persist(result);
          result = firstDao.getNext(result);
        }
      }
      catch (Exception e) {
        e.printStackTrace();
      }
      finally {
         transactions.end(); 
      }
    }
  }
 

Neither of these classes (the inner or outer) have unit tests, and it turns out that the inner class, MyCallable, has a bug in it. In the simplified version of the code that I've given you above, the bug isn't present.

So, lets assume you decide to fix the bug, and implement some unit tests for MyCallable. My question is this; How exactly would you go about writing unit tests for the MyCallable inner class?

My own solution was to first refactor MyCallable and ConcreteThread. MyCallable was made a public class in its own file, and ConcreteThread now passes in DAOs, Services and the Singleton as constructor arguments to MyCallable, rather than relying on an inner-class' access to it's private variables.

I then used EasyMock heavily in the Unit tests to mock those dependencies and verify that they were being called in the manner I expected.

A consequence of all this is that the code for MyCallable is somewhat larger than it was. As it no longer has access to the private variables in ConcreteThread, ConcreteThread must pass them in as arguments in the constructor, and MyCallable sets them as private variables.

Do you think that this was the wrong approach? That perhaps in by performing this sort of refactoring I have broken encapsulation and added unnecessary boilerplate to the code base? Would you have used reflection in the tests instead?

---

A consequence of all this is that the code for MyCallable is somewhat larger than it was. As it no longer has access to the private variables in ConcreteThread, ConcreteThread must pass them in as arguments in the constructor, and MyCallable sets them as private variables.

That's a good consequence, MyCallable is no longer dependent on changes in ConcreteThread.

I think the question and answer are quite subjective, but I think you followed the SOLID principle in the refactoring (which is a good thing).

And if you can, make MyCallable package protected, not public :)

---

To me it looks like, The inner class is an implementation detail of the outer class.

So I pose this question, can you demonstrate the bug by writing a failing unit test for ConcreteThread.Go() ? What should be different once you make the change to the inner class - what would be the externally visible change ? Once you figure that out - you'll be on your way.