Is doing a lot in constructors bad? [closed]

Making all fields final is in general a good idea, but sometimes I find myself doing everything in the constructor. Recently I ended up with a class doing actually everything in the constructor, including reading a property file and accessing a database.

On one hand, this is what the class is for, it encapsulates the data read and I like creating objects completely initialized. The constructor is not complicated at all as it delegates most of the work, so it looks fine.

On the other hand, it feels a bit strange. Moreover, in this talk at about 17:58 there are good reasons for not doing much work in constructor. I think I can eliminate the problem by passing appropriate dummies as constructor arguments.

The question remains: Is doing a lot of work (or even all the work) in constructors bad?

Solution 1:

I think that "Doing work in the constructor" is okay...

... as long as you don't violate Single Responsibility Principle (SRP) and stick to using Dependency Injection (DI).

I have been asking myself this question too lately. And the motivation against doing work in the constructor that I have found are either:

• It makes it hard to test
  • All examples I have seen have been where DI wasn't used. It wasn't actually the fault of the constructor doing actual work.
• You might not need all the results that your constructor calculates, wasting processing time and it's hard to test in isolation.
  • This is basically a violation of SRP, not a fault of the constructor doing work per say.
• Old compilers have/had trouble with exceptions thrown in constructors, hence you shouldn't do anything other than assign fields in constructors.
  • I don't think it's a good idea to write new code taking historical compiler deficiencies into account. We might as well do away with C++11 and all that is good all together if we do.

My opinion is that...

... if your constructor needs to do work for it to adhere to Resource Acquisition Is Initialization (RAII) and the class does not violate SRP and DI is properly used; Then doing work in the constructor is A-Okay! You can even throw an exception if you'd like to prevent usage of a class object whose initialization failed completely instead of relying on the user to check some return value.

Solution 2:

This is a very open-ended question, so my answer will try to be as general as possible...

Doing work in constructors isn't as "bad" as it used to be years ago when exception handling wasn't as prevalent and evolved as it is today. You'll notice that the Google Tech talk primarily looks at constructors from a Testing perspective. Constructors have been historically very very difficult to debug so the speaker is correct that doing as little as possible in a constructor is better.

With that said, you'll notice that he's also touching on dependency injection/the provider pattern which is notorious for complicating constructors. In such a case, leaving ONLY provider/DI code in the constructor is preferred. Again, the answer depends on what patterns you're using and how your code "fits" together.

The entire point of using a constructor is to create a neat object that can be used immediately; i.e. new Student("David Titarenco", "Senior", 3.5). There's no need to do david.initialize() as it would be entirely silly.

Here's some of my production code, for example:

    Config Conf = new Config();
    Log.info("Loading server.conf");
    Conf.doConfig();

In the above case, I decided not to do anything with the constructor (it's empty) but have a doConfig() method that does all the disk i/o; I've often thought that the doConfig() method is just pointless and I should do everything in the constructor. (I only check out the config file once, after all.)

I think that it's entirely dependent on your code and you shouldn't think that putting "stuff" in your constructor is a bad thing. That's what constructors are for! Sometimes we get carried away with OOP (getThis, setThat, doBark) when really all a class needs to do is a load a config file. In such cases, just put everything in the constructor and call it a day!

Solution 3:

I faced the following problems when I put too much code into the constructor:

• It was hard to write unit tests for other methods of that class, because it wanted to do a lots of things in the constructor, thus, I had to set up a lots of valid things or at least mocks (DB, file, whatever) for the simplest unit tests.
• It was hard to write unit tests for the constructor itself. Anyway, putting a lots of code with diversified responsibilites into one block is even a bad idea. (Single Responsibility Principle.)
• For the former reasons, it was hard to use that class. For example, it completely prevented me to implement some deferred init methods, because it needed everything in the moment of invoking the constructor. Ok, I could write the lazy init method into the constructor, nice.
• Sooner or later I realized that it had sense to reuse some code parts which are placed in the constructor. Well, when I first wrote the constructor I also thought that those code parts will be used just there, forever.
• When I wanted to extend that class and insert some logic before or into the super constructor's logic, it simply didn't work because the first thing to do in the extended class' constructor is to invoke the super's one.

So yes, doing a lot in constructor is a bad idea in my opinion.

Generally I just put some field initializations into the constructor and make an init method to invoke when everybody is on board.