Do I suffer from encapsulation overuse?

Question

I have noticed something in my code in various projects that seems like code smell to me and something bad to do, but I can't deal with it.

While trying to write "clean code" I tend to over-use private methods in order to make my code easier to read. The problem is that the code is indeed cleaner but it's also more difficult to test (yeah I know I can test private methods...) and in general it seems a bad habit to me.

Here's an example of a class that reads some data from a .csv file and returns a group of customers (another object with various fields and attributes).

public class GroupOfCustomersImporter {
    //... Call fields ....
    public GroupOfCustomersImporter(String filePath) {
        this.filePath = filePath;
        customers = new HashSet<Customer>();
        createCSVReader();
        read();
        constructTTRP_Instance();
    }

    private void createCSVReader() {
        //....
    }

    private void read() {
        //.... Reades the file and initializes the class attributes
    }

    private void readFirstLine(String[] inputLine) {
        //.... Method used by the read() method
    }

    private void readSecondLine(String[] inputLine) {
        //.... Method used by the read() method
    }

    private void readCustomerLine(String[] inputLine) { 
        //.... Method used by the read() method
    }

    private void constructGroupOfCustomers() {
        //this.groupOfCustomers = new GroupOfCustomers(**attributes of the class**);
    }

    public GroupOfCustomers getConstructedGroupOfCustomers() {
        return this.GroupOfCustomers;
    }
}

As you can see the class has only a constructor which calls some private methods to get the job done, I know that's not a good practice in general but I prefer to encapsulate all the functionality in the class instead of making the methods public in which case a client should work this way:

GroupOfCustomersImporter importer = new GroupOfCustomersImporter(filepath)
importer.createCSVReader();
read();
GroupOfCustomer group = constructGoupOfCustomerInstance();

I prefer this because I don't want to put useless lines of code in the client's side code bothering the client class with implementation details.

So, Is this actually a bad habit? If yes, how can I avoid it? Please note that the above is just a simple example. Imagine the same situation happening in something a little bit more complex.

Answer 1

I think you are actually on the right track as far as wanting to hide implementation details from the client. You want to design your class so that the interface that the client sees is the simplest and most concise API that you can come up with. Not only will the client not "be bothered" with implementation details, but you will also be free to refactor underlying code without worrying that callers of that code must be modified. Reducing the coupling between different modules has some real benefit and you should definitely strive for that.

So if you follow the advice I just provided, you will end up with something you already noticed in your code, a whole bunch of logic which remains hidden behind a public interface and isn't easily accessible.

Ideally, you should be able to unit test your class against its public interface only and if it has external dependencies, you might need to introduce fake/mock/stub objects to isolate the code being tested.

However, if you do this and you still feel that you cannot easily test every part of the class, then it is very likely that your one class is doing too much. In the spirit of SRP principle, you can follow what Michael Feathers calls "Sprout Class Pattern" and extract a chunk of your original class into a new class.

In your example you have an importer class that is also responsible for reading a text file. One of your options is to extract a chunk of file reading code into a separate InputFileParser class. Now all those private functions become public and therefore easily testable. At the same time the parser class is not something that is visible to external clients (mark it "internal", don't publish header files or simply don't advertise it as part of your API) as those will continue to use the original importer whose interface will continue to remain short and sweet.

Answer 2

As an alternative to putting a lot of method calls into your class constructor - which I agree is a habit to generally avoid - you could instead create a factory method to handle all of the extra initialization stuff that you don't want to bother the client side with. This would mean you would be exposing a method to look something like your constructGroupOfCustomers() method as public, and using it as either a static method of your class:

GroupOfCustomersImporter importer = 
    new GroupOfCustomersImporter.CreateInstance();

or as a method of a separate factory class perhaps:

ImporterFactory factory = new ImporterFactory();
GroupOfCustomersImporter importer = factory.CreateGroupOfCustomersImporter();

Those are the just the first couple of options thought of straight off the top of my head.

It's also worth considering that you're instincts are trying to tell you something. When your gut tells you the code is starting to smell, it probably smells, and it's better to do something about it before it reeks! Sure on the surface there may be nothing intrinsically wrong with your code per-se, however a quick recheck and a refactor will help to settle your thoughts about the issue so you can move on to other stuff confidently without worrying if you're building a potential house of cards. In this particular case, delegating some of the constructor's responsibilities to - or to be called by - a factory ensures you can exercise greater degree of control of the instantiation of your class and of it's potential future descendants.

Answer 3

Adding my own answer, since it ended up too long for a comment.

You are absolutely right that encapsulation and testing are pretty contrary - if you hide almost everything, it's hard to test.

You could, however, make the example more testable by giving a stream rather than filename - that way you just supply a known csv from memory, or a file if you want. It will also make your class more robust when you need to add http support ;)

Also, look into using lazy-init:

public class Csv
{
  private CustomerList list;

  public CustomerList get()
  {
    if(list == null)
        load();
     return list;
  }
}

Answer 4

The constructor should not do too much except initializing some variables or the state of the object. Doing too much in constructor can raise runtime exceptions. I would try avoiding client doing somenthing like this:

MyClass a;
try {
   a = new MyClass();
} catch (MyException e) {
   //do something
}

Instead of doing:

MyClass a = new MyClass(); // Or a factory method
try {
   a.doSomething();
} catch (MyException e) {
   //do something
}