9 Answers
9

I think the first one will give you the ability to create a config object elsewhere and pass it to ExampleA. If you need Dependency Injection, this can be a good thing because you can ensure that all intances share the same object.

On the other hand, maybe your ExampleArequires a new and clean config object so there could be cases where the second example is more appropriate, such as cases where each instance could potentially have a different configuration.

So, A is good for unit testing, B is good for other circumstances... why not have both? I often have two constructors, one for manual DI, one for normal usage (I use Unity for DI, which will generate constructors to fulfill its DI requirements).
–
Ed WoodcockJan 19 '12 at 10:17

3

@EdWoodcock it's not really about unit testing vs other circumstances. The object either requires the dependency from outside or it manages it on the inside. Never both/either.
–
MattDaveyJan 19 '12 at 10:36

Usually if the behavior of Example class depends on the config you want to be able to test it without saving/modifying the internal state of the class instance (i.e. you would want to have simple tests for happy path and for wrong config without modifying the property/memeber of Example class).

I've had this exact same discussion with our architecture team recently and there are some subtle reasons to do it one way or the other. It mostly comes down to Dependency Injection (as others have noted) and whether or not you truly have control over the creation of the object that you new in the constructor.

In your example, what if your Config class:

a) has non-trivial allocation, such as coming from a pool or factory method.

b) could fail to be allocated. Passing it into the constructor neatly avoids this issue.

The answer below is wrong, but I'll keep it for others to learn from it (see below)

In ExampleA, you can use the same Config instance across multiple classes. However, if there should be only one Config instance within the entire application, consider applying the Singleton pattern on Config to avoid having multiple instances of Config. And if Config is a Singleton, you could do the following instead:

In ExampleB, on the other hand, you'll always get a separate instance of Config for each instance of ExampleB.

Which version you should apply really depends on how the application will handle instances of Config:

if each instance of ExampleX should have a separate instance of Config, go with ExampleB;

if each instance of ExampleX will share one (and only one) instance of Config, use ExampleA with Config Singleton;

if instances of ExampleX may use different instances of Config, stick with ExampleA.

Why converting Config into a Singleton is wrong:

I must admit that I only learned about the Singleton pattern yesterday (reading the Head First book of design patterns). Naively I went about and applied it for this example, but as many has pointed out, one way are another (some have been more cryptic and only said "You're doing it wrong!"), this is not a good idea. So, to prevent others from making the same mistake that I just did, here follows a summary of why the Singleton pattern can be harmful (based on the comments and what I found out googling it):

If ExampleA retrieves its own reference to the Config instance, the classes will be tightly coupled. There will be no way of having an instance of ExampleA to use a different version of Config (say some subclass). This is horrible if you want to test ExampleA using a mock-up instance of Config as there is no way to provide it to ExampleA.

The premise of there will be one, and only one, instance of Config maybe holds now, but you cannot always be sure that the same will hold in the future. If at some later point it turns out that multiple instances of Config will be desirable, there is no way of achieving this without rewriting the code.

Even though the one-and-only-one instance of Config is maybe true for all eternity, it could happen that you want to be able to use some subclass of Config (while still having only one instance). But, since the code directly gets the instance via getInstance() of Config, which is a static method, there is no way to get the subclass. Again, code must be rewritten.

The fact that ExampleA uses Config will be hidden, at least when just viewing the API of ExampleA. This may or may not be a bad thing, but personally I feel that this feels like a disadvantage; for instance, when maintaining, there is no simple way of finding out which classes will be affected by changes to Config without looking into the implementation of every other class.

Even if the fact that ExampleA uses a SingletonConfig is not a problem in itself, it may still become a problem from a testing point of view. Singleton objects will carry state which will persist until the termination of the application. This can be a problem when running unit tests as you want one test to be isolated from another (i.e. that having executed one test should not affect the outcome of another). To fix this, the Singleton object must be destroyed between each test run (potentially having to restart the whole application), which may be time-consuming (not to mention tedious and annoying).

Having said this, I'm glad that I made this mistake here and not in the implementation of a real application. In fact, I was actually considering rewriting my latest code to use the Singleton pattern for some of the classes. Although I could easily revert the changes (everything is stored in an SVN, of course), I would still have wasted time doing it.

The simplest thing to do is to couple ExampleA to Config. You should do the simplest thing, unless there is a compelling reason to do something more complex.

One reason for decoupling ExampleA and Config would be to improve the testability of ExampleA. Direct coupling will degrade the testability of ExampleA if Config has methods that are slow, complex, or rapidly evolving. For testing, a method is slow if it runs more than a few microseconds. If all the methods of Config are simple and fast, then I would take the simple approach and directly couple ExampleA to Config.

Your first example is an example of the Dependency Injection pattern. A class with an external dependency is handed the dependency by constructor, setter, etc.

This approach results in loosely-coupled code. Most people think that loose coupling is a good thing, because you can easily substitute the config in the cases where one particular instance of an object needs to be configured differently from the others, you can pass in a mock config object for testing, and so on.

The second approach is closer to the GRASP creator pattern. In this case, the object creates its own dependencies. This results in tightly coupled code, this can limit the flexibility of the class and make it more difficult to test. If you need one instance of a class to have a different dependency from the others, your only option is to subclass it.

It can be the appropriate pattern, however, in cases where the lifespan of the depended-on object is dictated by the lifetime of the dependant object, and where the depended-on object is not used anywhere outside of the object that depends on it. Normally I'd advise DI to be the default position, but you don't have to rule out the other approach entirely as long as you're aware of its consequences.

If your class doesn't expose the $config to external classes, then I'd create it inside the constructor. This way, you're keeping your internal state private.

If the $config requires requires its own internal state to be set properly (e.g., needs a database connection or some internal fields initialized) before use, then it makes sense to defer the initialization to some external code (possibly a factory class) and inject it in the constructor. Or, as others have pointed out, if it needs to be shared among other objects.