Is Doing a Lot in Constructors Bad

Is doing a lot in constructors bad?

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.

How much work should be done in a constructor?

Historically, I have coded my constructors so that the object is ready to use once the constructor method is complete. How much or how little code is involved depends on the requirements for the object.

For example, let's say I need to display the following Company class in a details view:

public class Company
{
    public int Company_ID { get; set; }
    public string CompanyName { get; set; }
    public Address MailingAddress { get; set; }
    public Phones CompanyPhones { get; set; }
    public Contact ContactPerson { get; set; }
}

Since I want to display all the information I have about the company in the details view, my constructor will contain all of the code necessary to populate every property. Given that is a complex type, the Company constructor will trigger the execution of the Address, Phones and Contact constructor as well.

Now, if I am populating a directory listing view, where I may only need the CompanyName and the main phone number, I may have a second constructor on the class that only retrieves that information and leaves the remaining information empty, or I may just create a separate object that only holds that information. It really just depends on how the information is retrieved, and from where.

Regardless of the number of constructors on a class, my personal goal is to do whatever processing is necessary to prepare the object for whatever tasks may be imposed upon it.

Is it bad practice to populate lots of data in a constructor?

If everything belogns to the state of the created object, there is nothing "wrong" with that.

However, from an architectual point of view you should have some place where all the businesscode runs. And this is the place, where you issue the database connection and read the data. You could even write a seperate class for performing the population task.

Reason:

There might be some point where you wanna replace the population mechanism by something else. For example in a Unit Test you might want to populate the data with some hard coded test data, so you don't need a database for the unit test.

Is there anything wrong with taking immediate actions in constructors?

if the code in the constructor is part of creating and initializing the object for use, then I would put it there, but thats me personally, some people may disagree

however, it looks like what you are doing is not intended for building the object/class but doing some other process. this is bad, and should be done in a separate method.

Keep the constructor for construction.

Flaw: Constructor does Real Work

Concepts that urge you to keep your constructors light weight:

• Inversion of control (Dependency Injection)
• Single responsibility principle (as applied to the constructor rather than a class)
• Lazy initialization
• Testing
• K.I.S.S.
• D.R.Y.

Links to arguments of why:

• How much work should be done in a constructor?
• What (not) to do in a constructor
• Should a C++ constructor do real work?
• http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/

If you check the arguments in the constructor that validation code can't be shared if those arguments come in from any other source (setter, constructor, parameter object)

If you fill values into the collection or generate the information strings in the constructor that code can't be shared with other constructors you may need to add later.

In addition to not being able to be shared there is also being delayed until really needed (lazy init). There is also overriding thru inheritance that offers more options with many methods that just do one thing rather then one do everything constructor.

Your constructor only needs to put your class into a usable state. It does NOT have to be fully initialized. But it is perfectly free to use other methods to do the real work. That just doesn't take advantage of the "lazy init" idea. Sometimes you need it, sometimes you don't.

Just keep in mind anything that the constructor does or calls is being shoved down the users / testers throat.

EDIT:

You still haven't accepted an answer and I've had some sleep so I'll take a stab at a design. A good design is flexible so I'm going to assume it's OK that I'm not sure what the information strings are, or whether our object is required to represent a set of numbers by being a collection (and so provides iterators, size(), add(), remove(), etc) or is merely backed by a collection and provides some narrow specialized access to those numbers (such as being immutable).

This little guy is the Parameter Object pattern

/** Throws exception if sign of endValue - startValue != stepSize */
ListDefinition(T startValue, T endValue, T stepSize);

T can be int or long or short or char. Have fun but be consistent.

/** An interface, independent from any one collection implementation */
ListFactory(ListDefinition ld){
    /** Make as many as you like */
    List<T> build();
}

If we don't need to narrow access to the collection, we're done. If we do, wrap it in a facade before exposing it.

/** Provides read access only.  Immutable if List l kept private. */
ImmutableFacade(List l);

Oh wait, requirements change, forgot about 'information strings'. :)

/** Build list of info strings */
InformationStrings(String infoFilePath) {
     List<String> read();
}

Have no idea if this is what you had in mind but if you want the power to count line numbers by twos you now have it. :)

/** Assuming information strings have a 1 to 1 relationship with our numbers */
MapFactory(List l, List infoStrings){
    /** Make as many as you like */
    Map<T, String> build();
}

So, yes I'd use the builder pattern to wire all that together. Or you could try to use one object to do all that. Up to you. But I think you'll find few of these constructors doing much of anything.

EDIT2
I know this answer's already been accepted but I've realized there's room for improvement and I can't resist. The ListDefinition above works by exposing it's contents with getters, ick. There is a "Tell, don't ask" design principle that is being violated here for no good reason.

ListDefinition(T startValue, T endValue, T stepSize) {
    List<T> buildList(List<T> l);
}

This let's us build any kind of list implementation and have it initialized according to the definition. Now we don't need ListFactory. buildList is something I call a shunt. It returns the same reference it accepted after having done something with it. It simply allows you to skip giving the new ArrayList a name. Making a list now looks like this:

ListDefinition<int> ld = new ListDefinition<int>(3, 1, -1);
List<int> l = new ImmutableFacade<int>(  ld.buildList( new ArrayList<int>() )  );

Which works fine. Bit hard to read. So why not add a static factory method:

List<int> l = ImmutableRangeOfNumbers.over(3, 1, -1);

This doesn't accept dependency injections but it's built on classes that do. It's effectively a dependency injection container. This makes it a nice shorthand for popular combinations and configurations of the underlying classes. You don't have to make one for every combination. The point of doing this with many classes is now you can put together whatever combination you need.

Well, that's my 2 cents. I'm gonna find something else to obsess on. Feedback welcome.

Having lots of parameters in a constructor

1. In general, if you find yourself have too many parameters, that means you don't have enough low-level classes. In your case, you could have a class Name { /* fname, lname, initial, */ } and perhaps a class Contact { /* email, phone */ }

2. When you extend your class, have the constructor take the base as one parameter, and then add the new extra parameters. (You probably mean Employee will extend Person, rather than vice versa, so public Employee (Person person, Company company, String employeeId) { super(person); this.company = company; this.employeeId = employeeId; }

Good question!

How much work should constructor of my class perform?

The only truly golden unbreakable rule is that the class must be in a valid, consistent, state after the constructor has executed.

You can choose to design the class so that it is in some kind of "empty"/"inactive" state after the constructor has run, or you can put it directly into the "active" state that it is intended to be in.

Generally, it should be preferred to have the constructor construct your class. Usually, you wouldn't consider a class fully "constructed", until it's actually ready to be used, but exceptions do exist.
However, keep in mind that in RAII, one of the key ideas is that the class shouldn't exist unless it is ready, initalized and usable. That's why its destructor does the cleanup, and that's why its constructor should do the setup.

Again, exceptions do exist (for example, some RAII objects allow you to release the resource and perform cleanup early, and then have the destructor do nothing.)
So at the end of the day, it depends, and you'll have to use your own judgment.

Think of it in terms of invariants. What can I rely on if I'm given an instance of your class? The more I can safely assume about it, the easier it is to use. If it might be ready to use, and might be in some "constructed, but not initialized" state, and might be in a "cleaned up but not destroyed" state, then using it quickly becomes painful.

On the other hand, if it guarantees that "if the object exists, it can be used as-is", then I'll know that I can use it without worrying about what was done to it before.

It sounds like your problem is that you're doing too much in the constructor.

What if you split the work up into multiple smaller classes? Have the codec be initialized separately, then I can simply pass the already-initialized codec to your constructor. And all the authentication and cryptography stuff and whatnot could possibly be moved out into separate objects as well, and then simply passed to "this" constructor once they're ready.

Then the remaining constructor doesn't have to do everything from scratch, but can start from a handful of helper objects which are already initialized and ready to be used, so it just has to connect the dots.

What (not) to do in a constructor

Complex logic and constructor do not always mix well, and there are strong proponents against doing heavy work in a constructor (with reasons).

The cardinal rule is that the constructor should yield a fully usable object.

class Vector
{
public:
  Vector(): mSize(10), mData(new int[mSize]) {}
private:
  size_t mSize;
  int mData[];
};

It does not mean a fully initialized object, you may defer some initialization (think lazy) as long as the user does not have to think about it.

class Vector
{
public:
  Vector(): mSize(0), mData(0) {}

  // first call to access element should grab memory

private:
  size_t mSize;
  int mData[];
};

If there is heavy work to be done, you might choose to proceed with a builder method, that will do the heavy work prior to calling the constructor. For example, imagine retrieving settings from a database and building a setting object.

// in the constructor
Setting::Setting()
{
  // connect
  // retrieve settings
  // close connection (wait, you used RAII right ?)
  // initialize object
}

// Builder method
Setting Setting::Build()
{
  // connect
  // retrieve settings

  Setting setting;
  // initialize object
  return setting;
}

This builder method is useful if postponing the construction of the object yields a significant benefit. From example if the objects grab a lot of memory, postponing the memory acquisition after tasks that are likely to fail may not be a bad idea.

This builder method implies Private constructor and Public (or friend) Builder. Note that having a Private constructor imposes a number of restrictions on the usages that can be done of a class (cannot be stored in STL containers, for example) so you might need to merge in other patterns. Which is why this method should only be used in exceptional circumstances.

You might wish to consider how to test such entities too, if you depend on an external thing (file / DB), think about Dependency Injection, it really helps with Unit Testing.

Any problem with doing the main work of a class in its constructor?

In your example I would make a static method GetDBMSSpecInfo(java.sql.Connection conn) that will return an instance of DBMSSpecInfo object or null if something goes wrong (in case you don't want to throw exceptions).

The DBMSSpecInfo object for me should not contain nothing more than get properties: MaxIndexSize, MaxTableSize, DefaultSchema, etc.

And I would make the constructor of this object private so that instances can only be created from the static method.

---

Related Topics