1

I am trying to determine the best way to write testable code. Here's my code

class FileReader {

    private FileInputStream input;

    public FileReader(FileInputStream input) {
        this.input = input;
    }

    public void read() throws IOException {
        Row row = readHeaderRow();
        Row[] rows = readOtherRowsBasedOnHeader(row);
        doSomethingElse(rows);
    }

    private void readHeaderRow() {
        //..
    }
    private void readOtherRowsBasedOnHeader(Row row) {
        //..
    }
    private void doSomethingElse(Row[] rows) {
        //..
    }
}

as you can see from above, only the read() method is public. Rest of the methods are private. Should I just leave private methods out from testing? Or does it make sense to make all methods public and do what read() is doing in the calling code?

Jay
  • 2,394
  • 11
  • 54
  • 98
  • 2
    You should not make methods public or private for the sake of writing testable code. You can always use Reflection to test private methods of your class. – vk239 Dec 15 '15 at 20:11
  • Do testing frameworks like Junit/TestNG support such reflection based testing out of the box? – Jay Dec 15 '15 at 20:16
  • 1
    Here is a link which has an article on testing private methods using Junit and reflection - http://www.jroller.com/CoBraLorD/entry/junit_testing_private_fields_and – vk239 Dec 15 '15 at 20:18

4 Answers4

3

My view is that you should only test public methods. The usage of the private methods will be tested regardless by calls from the public method. It will also make internal refactoring much easier without changing the test.

What you want to test is that the class fulfills its contract, ie. the public methods, no matter how it looks on the inside.

nedenom
  • 405
  • 4
  • 14
2

If the testing of the public method covers all the code in the private methods, is safe to consider the class tested

snovelli
  • 5,804
  • 2
  • 37
  • 50
2

If logic of private methods (readHeaderRow, readOtherRowsBasedOnHeader, ...) is complex and require separate tests I suggest to implement FileReader as a composition of smaller classes. It would be something like:

class FileReader {

private FileHeaderReader headerReader = new FileHeaderReader();
private FileOtherReader otherReader = new FileOtherReader();
//....
private FileInputStream input;

public FileReader(FileInputStream input) {
    this.input = input;
}

public void read() throws IOException {
    Row row = headerReader.read();
    Row[] rows = otherReader.read(row);
    //do something else
}  
}

class FileHeaderReader {
    public Row read() {...}
}
//....

Then you can write tests that precisely test logic of each part/class. You can also think about injecting FileHeaderReader into FileReader so this classes are not tightly coupled.

Community
  • 1
  • 1
Paweł Adamski
  • 3,285
  • 2
  • 28
  • 48
  • I thought about this, but I figured if I keep doing this, I would end up a class explosion of sorts. – Jay Dec 15 '15 at 20:32
  • It is not a general solution. Just a one of many. Always use your mind to decide which way will be the best. Regardless that don't be afraid of having to many classes and remember that object composition usually leads to more testable code. – Paweł Adamski Dec 15 '15 at 20:42
2

I see two viable options.

  1. You test the read(), so your test should cover the conditions and states changed in the private methods. It really depends on the responsibility and complexity of those methods whether you want to do that.
  2. You find out those private methods violate SRP and separate them so they are now public in a different class.
David Baak
  • 944
  • 1
  • 14
  • 22