3

When you browse for the phrase "constructors must not do work", then in various blog posts you will find the advice to not let the constructor do work. Despite this, I am having some trouble understanding why this is the case. Additionally, this popular post suggests to take such advice with a grain of salt.

I have an example of two implementations of the same situation. In the situation, a AFactory has a method createA, using a B. A needs a query result, that B produces. There are two ways to implement this:

Example 1:

class AFactory {
    public function createA(B $b): A {
        return new A($b->getQueryResult());
    }
}

class A {
    private $query_result;

    public function __construct(array $query_result) {
        $this->query_result = $query_result;
    }

    public function doFooWithQueryResult() {
        // Do something with query result
    }

    public function doBarWithQueryResult() {
        // Do something with query result
    }
}

In the first example, the factory fetches the query result and passes it to A's constructor. A then merely assigns the query result to the corresponding class property. However, there is one problem here: A does not verify if the query result is a valid data structure, i.e. an actual query result suited for A. It does not know where it came from. The responsibility for this validation has now leaked to the AFactory, and A has become very tightly coupled to AFactory. The other implementation resolves this issue, but then the constructor performs work. And apparently that is bad.

Example 2:

class AFactory {
    public function createA(B $b): A {
        return new A($b);
    }
}

class A {
    private $query_result;

    public function __construct(B $b) {
        $this->query_result = $b->getQueryResult();
    }

    public function doFooWithQueryResult() {
        // Do something with query result
    }

    public function doBarWithQueryResult() {
        // Do something with query result
    }
}
  • Factory being coupled to its product (and product's dependencies) is ok - it handles concrete implementations after all. Factories should be located in infrastructure layer behind application boundry/abstraction/interface. Constructor validation is a bit defensive, but if your A is some general type with simple type dependencies then it's acceptable as well imo. Note that often structurally correct data might trun out to be invalid (www.example.com domain might not exist) and you'll find that out at "runtime" anyway. – shudder Jan 18 '17 at 16:22
  • Doing work in a constructor can be fine. But it is often (in my experience, usually) a code smell. For instance, you might setup some things and then fail the constructor due to some sort of error. This could leave some resources in a dirty state (their memory may never be reclaimed). If you call a member function from the constructor, it might expect certain things to be setup which aren't setup yet. In java, if you pass a reference to this outside the constructor, you may have just written a memory leak (especially if an exception is thrown afterwards). – Shadow Man Jan 18 '17 at 17:08
  • IMO, it is much easier to break things down into small manageable chunks (functions) with correct cleanup procedures on failure, if you do it via a factory method, rather than in the constructor. It is also (again, IMO) easier to maintain such a factory method than it would be to maintain such a complicated constructor. – Shadow Man Jan 18 '17 at 17:10
  • I guess you don't want work in the constructor otherwise you would limit the class scope. It's like you are pretending it's a class but in reality is just a complex function. – borracciaBlu Jan 18 '17 at 21:10
4

You wrote

A does not verify if the query result is a valid data structure, i.e. an actual query result suited for A.

directly followed by

It does not know where it came from

But these are two different things! A does not not need to "know where the data is coming from", but it can of course validate its input:

class A {
    private $query_result;

    public function __construct(array $query_result) {
        // makes tests, throws an exception if $query_result is not valid
        validateInput($query_result);  

        $this->query_result = $query_result;
    }
    // ...
}

IMHO input validation in the constructor is not counting as "actual work" which should be done somewhere else. Nevertheless the constructor does not need to call getQueryResult. That will keep A decoupled from B, which makes testing and reuse much easier.

  • Your answer fails to address the reason why a constructor must not do work though. Anyway, if I were to validate that input, then the whole query result would have to be validated, i.e. all returned rows and their columns should be present in the query result. Isnt that a lot of work both for the programmer and for the program to conduct? – user2180613 Jan 18 '17 at 14:05
  • 1
    @user2180613, you failed to mention "the whole query result would have to be validated, i.e. all returned rows and their columns should be present in the query result" in your question. If the query result is difficult to validate - for the program and programmer - then why did you even mention it in your question? – David Arno Jan 18 '17 at 15:05
  • @user2180613: the top answer in the link you gave already discussed it. It is not so much about not do "too much work", but not to do "the wrong kind of work", which is "work" requiring additional dependencies. In this case, the dependency of A from B can be avoided by following "tell don't ask". In your second example, A might omit the validation (if you are 100% sure $b->getQueryResult() always delivers a correct value), but for the price of having A and B tightly coupled. – Doc Brown Jan 18 '17 at 16:24
  • I agree that input validation is not counted as "work". In java, you might do the following in a constructor and it wouldn't be a code smell (IMO) this.someVar = Objects.requireNonNull(someVar); or if (rangeMin > rangeMax) throw new IllegalArgumentException("min exceeds max in range parameters");. – Shadow Man Jan 18 '17 at 17:14
0

What is A? This is the fundamental question in designing a class in OOP.

Most problems arise because of not asking this question at the outset and instead focusing on implementation details of the class. Your example gives me this impression.

Back to the question:

  • Ask the question before doing any details. In other words you need to define what the class A is. Don't move to implementation before finalizing this question. Typically, one is tempted to think that once one starts coding, the class will come together somehow. Avoid this temptation.
  • When asking this question, try to define a single, yes single responsibility for the class, not many. And this responsibility has to be clear not vague.
  • Once you have made the definition, then it becomes much easier to see what properties this class has, because properties stem from its definition. For example, if you decide that your class is a Vehicle, then it must have attributes such as body, engine, tires, steerings etc and actions such as move, stop, turn, speed up, slow down, etc.
  • It is important to do these steps before diving into coding, no matter how tempting that could be.

Now, to your question regarding constructors should not do the work:

My understanding for this rule (though not absolute as you have mentioned) is this: In your application, by the time you reach the point to construct an object of a class, the data for that construction should be already available and passed to constructor. For example, say in your web application, you gather user-entered data through a form to construct an object.

// Gather user information
email = email from form data
...

// Do data validation if necessary
if (name is valid)
if (email is valid) 
if (credit card is valid)
...

// Now ready to construct object
User user = new User(name, email, creditCard, ...)
...

Same thing goes if you need to get data from a database to be used for construction of an object, in you example A.

The idea is, in the context where you create an object, pre-construction work needs to be done there. For example, the above example could a Controller in a web application.

There is one thing that should be better done in the constructor, and that is the kind of work that logically is part of the definition of a class (Earlier explanation about the defining a class). For instance, in the web user example, if it is part of the business logic that each user be assigned a rank based on their credit limit, this kind of work is not suited in the controller, not perhaps in a service class, but in the business object itself. So, something like this could be done in constructor.

0

Class A shouldn't know what B does. Class A shouldn't even know what a query result looks like.

An improvement would be to have explicit named parameters - you know exactly what data you need to construct A - whether that comes from a query result or not.

I'd think about what could actually be wrong with the query result - it seems that if the query is wrong, this is more of an exceptional circumstance that should be treated as such - leading to "A" instances never being created.

More generally speaking as Doc Brown suggests, it's about reducing "coupling" - or how dependent a piece of code is on a different piece.

See here for a quick guide to some different ways code may be coupled. You'll see it's impossible to avoid in some way. You want your code to be as loosely coupled as possible so you can test and modify it independently.

If your constructor just takes named parameters - you don't need to do complicated work in it, except maybe assigning simple variables. The complicated work you do to you arrive at those parameters' values can then be ignored by A, or any dependency on A, making A really easy to unit test. That complicated work is also then a candidate for hiding behind an abstraction and unit tested too.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.