Is doing a lot in constructors bad?

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?

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

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

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][2] 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][1], 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)][3] and the class does not violate [SRP][1] and [DI][2] 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.

[1]: http://en.wikipedia.org/wiki/Single_responsibility_principle "Single Responsibility Principle (SRP)" [2]: http://en.wikipedia.org/wiki/Dependency_injection "Dependency Injection (DI)" [3]: http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization "Resource Aquisition Is Initialization (RAII)"

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!

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.

It usually is, if your object has a complicated creation algorithm you could probably simplify it using a Builder or a Factory. Specially if there are pre-conditions to be validated to build the object.

Once your start using Builders and Factories to build your objects they can validate the pre and post conditions and make sure clients of your code will only be able to access a fully initialized object and not a half-made one, you can even use the nowadays in vogue fluent interfaces to create your object and make it look cool ;D

new EmailMessage()
    .from("[email protected]")
    .to("[email protected]")
    .withSubject("Fluent Mail API")
    .withBody("Demo message")
    .send();

Obviously this isn't really your case, as this is not using a Builder, but it's much like something you could build to make your constructor do less work and make your code look simpler.

Having constructors and destructors in my opinion is good, but not to do too much work in them. Especially not file/database access unless its very very specific to the class. You want to keep your constructors/destructors light to keep your program feeling fluid. Sometimes like already you have come to a case where the constructor does essentially all the work. There is a way to make things lighter. The concept/paradigm is called lazy evaluation. The idea is to accept inputs and do nothing (e.g. in constructor) but make use of the inputs when you need a calculation requested.

Example: Lets say you have a class that reads a file, parses it and tells you info like the sum of all numbers in the file. You can do this all in the constructor. Using lazy evaluation you will merely open the file, and have a getTotalSum() function. When called it will do the parsing and give you the result. This way you can also have getBestFit() to get best fit line. Sometimes you dont want to get best fit and for some inputs you do. This way the user will not be waiting for the constructor to do the calculations before the user decides what to do.

another example: lets say you have a view that loads 20 images. But only 5 are shown, and the constructor takes an array of the images to show. You can load them all in the constructor, but from a users perspective this will feel slow at the beginning. Or you can load 1 "loading" picture and load 1 image at a time. And as the user scrolls load more of the images on a as shown/needed basis.

Of course 1 problem is that you find out of errors like invalid pictures later down the road instead of the constructor. You can always perform simple checks for yourself to pre-validate the input to some degree (e.g. check for correct password).