Work in the constructor anti-pattern

/**
* 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.
Advertisements

Author: tamasgyorfi

Senior software engineer, certified enterprise architect and certified Scrum master. Feel free to connect on Twitter: @tamasgyorfi

4 thoughts on “Work in the constructor anti-pattern”

  1. The only solution I find acceptable is the third one. The constructor’s responsibilty is to set up everything for the object, and leaving a way to have not properly initialized objects (eg. the dreaded ‘initialize’ method) leads to an extremely brittle design.

    The constructor itself could be mocked as well using Powermock, or the object construction phase can be abstracted away by a Factory.

    Also if a constructor is busy, it implicates that the class itself is busy too. So to summarize, a constructor which does *something* is not an anti-pattern, (empty constructors are an indicator of an anemic Domain, which is baaaaad), but a too busy constructor indicates a violation of SRP.

    1. Csongor, I’m happy you bring this up. And, in general, we agree that nr. 3 is the way to go, in like 85% of all cases. It definitely is the best one for the example in the post.

      However, there are some (rare) cases when you might find other possibilities useful too. Let’s say we encounter a not so nice socket implementation, that, in the constructor opens up the socket on one go. This is, of course not good, so we might want to factor out the opening logic to a new method and make that the caller’s responsibility to call it. That move would perfectly make sense. In this case, an extra SocketOpener dependency would make the design unnecessarily complicated and should be avoided, as it would create a fragmented logic. Of course, this is somewhat different situation from the one explained in the post.

      There are some even rarer cases when the @PostConstruct comes in awesomely handy, just ran into one myself a few weeks ago. Maybe it would worth its own post, we’ll see.

      1. In the rare case you described I think there’s a violation of SRP, even if the socket opening logic is extracted from the constructor to a method. What I’d propose when encountering such a problem is that the class which needs an open socket should be passed an open socket as a dependency, and the constructor should check whether the given socket is open.

        The problem with requiring the programmer to call a method after the object is constructed is that you create a chance for your class to be misused. A way to mitigate that problem is to check whether the socket is open in the preamble of each method, but that’s also a really complicated an error prone solution.

        I’m looking forward for the post in which you’ll explain why the @PostConstruct was handy.

        1. I don’t see it that way.

          The calling code may or may not need an open socket; it can decide it in runtime – hence an expensive operation is deferred.

          This does not hurt SRP, as opening a socket should really be Socket’s business. The same approach is used in case of javax.jms.Connection, where this interface explicitly decouples creation logic from opening logic through a start() method.

          Some Spring application context implementations also go with the same mindset, you have to manually initialize the app context.

          About misusing a class: sure, you may run into problems. Just like with any other class. (You can easily misuse a TreeSet if you try to call the add() method with objects that do not implement the Comparable interface and supply no Comparator instance). Things can go wrong, but classes we create will be used by other professionals, who are able to handle exceptions.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s