/**
* This post is intended to be a 101 quickie for the less experienced.
* It does not provide new or innovative ways of solving a certain problem,
* just summarizes a topic the way I see it.
*
**/

This is a topic many have already talked about (a lot), but it still pops up from time to time. Strangely enough, not only in case of junior developers.

The situation

Given a Constructor full of business logic; it opens files, sockets, creates message queues etc. Usually it looks something similar to the snipped below:

    private Config readValue;

    public FileBasedConfigProvider(String fileName) {
        //...
        try {
            Path path = Paths.get(fileName);
            byte[] configContent = Files.readAllBytes(path);

            ObjectMapper mapper = new ObjectMapper();
            readValue = mapper.readValue(configContent, Config.class);
        } catch (IOException e) {
            throw new IllegalStateException(e);
        }
        //...
    }

The problem

First of all, a class with a similar constructor is really hard to use and test. Just imagine working with such a monster, we have to supply it a valid file name, that file’s content has to be well formatted JSON, which can then be mapped to an object of type Config. And we haven’t even started using the config provider functionality (the one we need this class for). Also, there is no way of replacing the file reading or the object mapping logic with test doubles for easy unit testing. In short, there is really no way to easily create a new instance of such a class in a test harness or in production code.

Another problem is that constructors are not named code blocks; we can’t give a descriptive name to a constructor, like in case of methods. Let’s assume an exception is thrown from our long constructor; the stack trace:

Exception in thread “main” java.lang.IllegalStateException: java.nio.file.NoSuchFileException: config.json at FileBasedConfigProvider.<init>(FileBasedConfigProvider.java:23)  …

does not really help diagnose what/where went wrong. Ok line numbers can help, but still, is not as descriptive as, let’s say, readConfigurationFile().

Usually classes with such constructors do not conform to the Single Responsibility Principle either. Like in the case above: the constructor first needs to do the file reading and object mapping (config resolution, responsibility 1) so then it can supply config parameter values (responsibility 2).

All in all,  constructors are simply not meant to do busywork. That’s methods’ job.

The solution(s)

Keep the constructor clean (a.k.a. assignments only) and …

• Move the content of the constructor into a method. Don’t let the constructor call it (calling overridable methods from constructors is again a big problem), make that the client’s responsibility. This is somewhat one step forward, at least from testing perspective, as the method becomes overridable in the unit tests. The overriding method can do no work at all, return dummy/fake/mock answers and so we can bypass the IO operations that make the class hard to test. However, this solution is not a perfect one, as clients must not forget about calling the method. It does also not address multiple responsibilities and ease of use.
• If you are using an IoC container that can understand standard javax annotations, you can extract the work from the constructor (just right in the previous case) and annotate it with @javax.annotation.PostConstruct. That way by the time the object becomes available for use, the method will already have completed automatically. Client code does not need to call the initializer method, and you can still override it in your tests.
• Move the content of the constructor into its own class (in this case, ConfigReader, for example) and inject it as a constructor dependency. This way the config provider is doing only one thing (supplying config values) while the ConfigReader worries only about I/O stuff.