21

I answered a question (link) that I used a creation of the new object in another class' constructor, here the example:

class Person {
  public $mother_language;

  function __construct(){ // just to initialize $mother_language
    $this->mother_language = new Language('English');
}

And I got the comment from user "Matija" (his profile) and he wrote: You should never instantiate a new object inside object consturctor, dependencies should be pushed from outside, so anyone who uses this class knows what is this class dependent on!

Generally, I can agree with this, and I understand his point of view.

However, I used to do this this way very often, for example:

  • as the private properties other classes give me functionality that I can solve not duplicating the code, for example I can create a list (class implementing ArrayAccess interface) of objects), and this class would be used in another class, that has such a list of objects,
  • some classes use for example DateTime objects,
  • if I include (or autoload) dependant class, one should have no problem with errors,
  • because dependant objects can be very large number, passing all of them to the class constructor can be very long and not clear, example

    $color = new TColor('red'); // do we really need these lines?
    $vin_number = new TVinNumber('xxx');
    $production_date = new TDate(...);
    ...
    $my_car = new TCar($color, $vin_number, $production_date, ...............);
    
  • as I was "born" in Pascal, then in Delphi, I have some habits from there. And in Delphi (and FreePascal as its competitor) this practice is very often. For example, there is a TStrings class that handles array of strings, and to store them it does not use arrays but another class, TList, that provides some useful methods, while TStrings is only some kind of interface. The TList object is private declared and has no access from outside but the getters and setters of the TStrings.

  • (not important, but some reason) usually I am the one who uses my classes.

Please explain me, is it really important to avoid creating objects in constructors?

I've read this discussion but have still unclear mind.

Community
  • 1
  • 1
Voitcus
  • 4,463
  • 4
  • 24
  • 40
  • @Matija I'll read, thanks. I wanted to say I understand what you mean but I can't agree with allowing access to the class's properties from outside to anybody else. If I provide a class to someone, I think he should get working device, that he can construct and get all he needs. I don't want to make him create lots of additional objects that its purpose might be not clear or I want to keep it secret, that's why. And I don't want him to handle this property object outside my class, this is MY object and keep away from it. – Voitcus Apr 09 '13 at 13:08
  • @Voitcus That's why we have Dependency Injection Containers, that can construct Objects for us, with all their dependencies. So user of your system would not construct all objects, he would just say: $container->getService("Person"); And container will use Dependency Injection to construct Person object with all dependencies. I wrote nice little Dependency Injection Container for my MVC, you can check it out here: https://github.com/matijabozic/php_services – Matija Apr 09 '13 at 13:11
  • @Matija Please post it as the answer and so I could accept it – Voitcus Apr 09 '13 at 13:27
  • ouhhh I'm in a rush now, can you please accept answer from Schleis, he explained everything, except Dependency Injection Containers but that's simple concept. Accept Schleis answer, thanks! – Matija Apr 09 '13 at 13:30
  • @Matija Ok, thank you once again – Voitcus Apr 09 '13 at 13:30
  • Better: [Flaw: Constructor does Real Work (ca. Nov 2008; by Miško Hevery)](http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/) – hakre Aug 28 '13 at 22:12

7 Answers7

12

Yes it really is. You are then clear about what the object needs in order to be constructed. A large number of dependent objects being passed in is a code smell that perhaps your class is doing too much and should be broken in up in multiple smaller classes.

The main advantage of passing in dependent objects comes if you want to test your code. In your example, I cannot use a fake Language class. I have to use the actual class to test Person. I now cannot control how Language behaves to make sure that Person works correctly.

This post helps explain why this is a bad thing and the potential problems that it causes. http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/

UPDATE

Testing aside, passing in dependent objects also makes your code more explicit, flexible and extensible. To quote the blog post that I linked to:

When collaborator construction is mixed with initialization, it suggests that there is only one way to configure the class, which closes off reuse opportunities that might otherwise be available.

In your example, you can only create people that have "English" as a language. But what about when you are wanting to create someone who speaks "French". I can't define that.

As for creating the objects and passing them in, that is the whole purpose of the Factory pattern http://www.oodesign.com/factory-pattern.html. It would create the dependencies and inject them for you. So you would be able to ask it for the object that would be initialized in the manner that you want. The Person object should not have to decide what it needs to be.

Schleis
  • 41,516
  • 7
  • 68
  • 87
  • You don't mock objects by making a horrid mess of your code. You mock them by swapping out their type definitions for _alternative_ definitions. – Lightness Races in Orbit Apr 09 '13 at 12:43
  • 1
    But if the objects are instantiated in the constructor, you can't swap them at all. You have to use what is specified there. – Schleis Apr 09 '13 at 12:44
  • @Schleis: What total nonsense. You're creating an _object_ from the constructor, but that is not the place where you create the object's _type_. So the constructor includes `new T` -- great! Good! Now, elsewhere, change what comes between `class T {` and `}` to suit your testing needs. This is called _mocking_. – Lightness Races in Orbit Apr 09 '13 at 12:47
  • @LightnessRacesinOrbit: Can you provide an example of how to swap out the definition of a class so that the same constructor gets you a different object? The only way I could think of would be to change the file that would get included, which doesn't really work in an application with an autoloader, and it would be pretty messy. – Travesty3 Apr 09 '13 at 12:49
  • 1
    @Travesty3 `include('classes/language.php')` <-- just swap that line with `include('classes/test/otherlanguage.php')` et voila you have swapped your definition with almost no overhead. of course there are other ways to load class definitions in php but they are also easy to manage/manipulate so that other files are included instead of the real ones. – ITroubs Apr 09 '13 at 12:52
  • 1
    @LightnessRacesinOrbit Creating alternative definitions of your classes seems a good way to completely mess up your code. – arraintxo Apr 09 '13 at 12:54
  • @arraintxo: I don't see why. That's the standard method of creating _mocks_ for unit testing. Your production code is left completely intact, and you haven't made a total hash of encapsulation in the meantime. Testing is excellent but you should **never** make your code an abominable mess of poor, unencapsulated design purely to enable testing. – Lightness Races in Orbit Apr 09 '13 at 12:57
  • @ITroubs: And how would that work with an autoloader? Where do these `include`s occur? If there are multiple places (which would almost always be the case), you would have to make sure that *every* place that the include occurs gets changed, otherwise you'll have two different definitions of the same class, which is a fatal error. – Travesty3 Apr 09 '13 at 12:58
  • @Travesty3 then you just rename your actual class file and the test classfile. – ITroubs Apr 09 '13 at 13:01
  • 1
    @ITroubs: Are you suggesting to rename the class, or rename the file? Renaming the class will not work, since the code that you're testing is still creating an instance of the same class. If you're suggesting to rename the file, that won't make any difference...you'd still have to make sure all `include`s get changed, otherwise you're still redefining a class. – Travesty3 Apr 09 '13 at 13:08
  • @Travesty3 Why is that? If my class `language` is in the file `lanugage.php` and I rename just the `language.php` to anything else and rename my file containing some other `language` class for testing and i rename that exact file to `language.php` then php will load my test language. no further editing of the code needed.. – ITroubs Apr 09 '13 at 13:10
  • 2
    @ITroubs: Here is another problem with that strategy. What if you need to include one version of the class for one test, and then another version of the class for a different test? You would include two different definitions of the same class. – Travesty3 Apr 09 '13 at 13:16
  • 1
    @Schleis Thanks for this answer, I came here to write my answer, but you already explained everything. Thanks! – Matija Apr 09 '13 at 13:26
  • 2
    @Schleis Thank you. I found more clear explanations here http://misko.hevery.com/2008/07/08/how-to-think-about-the-new-operator/ – Voitcus Apr 09 '13 at 13:29
7

The original comment seems silly to me.

There's no reason to be afraid of classes that manage resources. Indeed, the encapsulation that classes provide is perfecty apt for this task.

If you restrict yourself to only ever pushing in resources from the outside, then what shall manage those resources? Spaghetti procedural code? Yikes!

Try to avoid believing everything you read on the internet.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • 3
    I the answer to the question in the discussion he mentiond has a real point. There might be some class that really takes time when you instanciate it. if then your class almost never or just in some verry rare cases uses that class then why instanciating it in the constructor? it would only be a waste of performance! – ITroubs Apr 09 '13 at 12:36
  • 3
    @ITroubs: How can you speak about performance yet ? Have you profiled things ? One should focus *first* on code clarity, not performance. – ereOn Apr 09 '13 at 12:38
  • @ereOn the only thing i said is that there are some special casses where instantiating in the constructor might be a pitfall. nothing more nothing less. – ITroubs Apr 09 '13 at 12:39
  • @ITroubs: Using a similar "logic", I could say that in some special cases wearing pants is a bad idea. But in the general case it's probably nicer for everyone. – ereOn Apr 09 '13 at 12:42
  • @ereOn of cours you could. Thus thinking about what you wear is a nice idea ;-) – ITroubs Apr 09 '13 at 12:43
  • 1
    @LightnessRacesinOrbit I agree that dependency injection best practices can lead to spaghetti code. However, the other benefits outweigh the negatives. If you want to simplify the spaghetti, implement a Factory or Builder as Schleis recommended. – Marty Penner Aug 15 '13 at 15:40
  • "*if you restrict yourself to only ever pushing in resources from the outside, then what shall manage those resources? Spaghetti procedural code? Yikes!*" This is a potential problem, agreed, but *only* when it is the problem you described, and that is certainly not the *only* scenario. what about pushing in those dependencies via non-spaghetti code? Like a really well structured framework which has good dependency management? – James Aug 09 '15 at 16:47
4

This is more a testability issue than a right/wrong thing, if you create an object on your constructor you will never be able to test your class independently.

If your class needs lots of injections on the constructor, you can allways use a factory method to inject those objects.

In this way, you will be free to mock any of those injected objects to really test your own class.

<?php

class Person {
  public $mother_language;

  // We ask for a known interface we don't mind the implementation
  function __construct(Language_Interface $mother_language) 
  {
    $this->mother_language = $mother_language
  }

  static function factory()
  {
    return new Person(new Language('English'));
  }
}

$person = Person::factory();
$person = new Person(new Language('Spanish'); // Here you are free to inject your mocked object implementing the Language_Interface
arraintxo
  • 484
  • 2
  • 12
  • 1
    `if you create an object on your constructor you will never be able to test your class independently.` What total nonsense. You can write `new T()` inside the constructor and still mock `T`, by swapping out its definition. Admittedly this is _far_ easier in C++ when you have fine control of the translation units linked into your executable, but it's not so much less the case here. And certainly making a general rule -- as the question seems to indicate -- that you should _never create objects in a constructor_ is ludicrous. – Lightness Races in Orbit Apr 09 '13 at 12:44
  • I'm not saying you should never do it. I just say that this difficults testing, I may be too categoric in that "never", but it will always be easier with injections. BTW, I don't even think this should be always done in this way... – arraintxo Apr 09 '13 at 12:51
  • If you were to have a factory, I would call it PersonFactory and put it in a different class. I dislike having static methods. – Anyone Apr 10 '13 at 08:16
3

I think it depends on the situation. If you look at Doctrine 2 for example, it's recommended to put your values always to null and set your values in the constructor. This because Doctrine 2 skips instantiating the class when it's retrieved from your database.

It's perfectly acceptable to instantiate a new DateTime or ArrayCollection in there, you could even add default relational objects. I personally like Injecting your dependencies, keeps your code really easy to test and modify with little effort, but some things just make way more sense to instantiate in your constructor.

Anyone
  • 2,814
  • 1
  • 22
  • 27
2

Most of these answers seem reasonable. But I always do what you do to. If I am testing it, then I either have a setter or just set it to something (seeing that you "mother_language" is public).

But I personally think it depends on what you are doing. From the code that I am seeing posted by others, if your class instantiates objects, the constructor should always take some parameters.

What if I want to create an object that instantiates another object where that inner object instantiates another and so on? That seems like I will have a lot on my hands.

To me, that statement seems like he's saying, "don't use camelCase, use an under_score". I think you should do it whatever way works for you. After all, you do have your own way of testing, don't you?

Touch
  • 1,481
  • 10
  • 19
2

If you want to set a default, but improve flexibility/testability, why not do it like this?

class Person
{
    protected $mother_language;

    function __construct(Language $language = null)
    {
         $this->mother_language = $language ?: new Language('English');
    }
}

Some people still may not like it, but it is an improvement.

ryanwinchester
  • 11,737
  • 5
  • 27
  • 45
0

There is nothing wrong with constructing new objects inside a constructor. There are advantages to both approaches. Taking in all constructor dependencies through the constructor arguments does make your class more flexible, and this flexibility especially comes in handy when writing automated tests for your class. But that also creates a layer of indirection. It also makes the class less convenient to use in the average use case because there are more constructor parameters to think about. It also makes it harder to know exactly how the class will behave because you don't know exactly what will be passed in. It makes the class less simple.

Taking in all the constructor dependencies through parameters is one way of using something called "inversion of control". I agree it is important to be familiar with this technique and to be capable of using it, but promoting is as fundamentally superior is exactly backwards. Inversion of control is evil, but sometimes it is a necessary evil. Some people would say it is always necessary. I don't agree with that, but even if I did, I would say this means it is always a necessary evil. Inversion of control makes your code less straightforward in the ways described above. When people argue in favor of inversion of control, what they are really saying is, "Using code is bad". I'm not saying people are intentionally saying that, because if they did they might not promote it. I'm saying there are non-obvious ways in which those concepts are saying the same thing, and hopefully by realizing they are saying this, some people will rethink it. I outline the reasons for this claim in my article "Using code is bad".

still_dreaming_1
  • 8,661
  • 6
  • 39
  • 56