Design Patterns – Do I Suffer from Encapsulation Overuse?

clean codedesign-patternsencapsulationobject-oriented-design

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.

Best Answer

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.

Related Topic