2

These are two classes, the first one I inject an instance of type IEngine while the second one I inject owner's name, tickets and engine.

Version 1:

public class Car
{
    public Car(IEngine engine)
    {
        // do something here
    }

    public void PrintOwnerName()
    {
    }
}

Version 2:

public class Car
{
    // owner's name, tickets history of this car, and the engine.
    public Car(string ownerName, List<string> tickets, IEngine engine)
    {
    }

    public void PrintOwnerName()
    {
    }
}

I was wondering if the version 2 is bad design. If so, why is it bad?

  • Go watch this. You'll learn something. (I disagree with the note about the value of integration tests since I think they can be useful if you limit their scope, but that's minor to the talk.) (I know it might only seem tangentially related, but it should start giving you the right mindset for thinking about how to organize your design.) – jpmc26 Dec 8 '14 at 10:04
5

The decision which parameters a constructor should have is the same decision which parameters an arbitrary function should have - it should have exactly the parameters which are needed to create a specific, ideally easy to understand, abstraction. And if your abstraction of a car encapsulates exactly those three things, version 2 reflects that much better than version 1.

In your first version, you would have to provide owner name and tickets differently, probably by setting those properties afterwards. But if those properties must be always provided, it is IMHO a better design enforce this by constructor parameters.

One thing you have to care for is that the number of parameters passed through the constructor does not grow to a point where the code becomes hard to understand. For example, when you have a lot more parameters for your car like vendor, color, number of seats, number of doors, list of previous owners, needed driving licence for this type of car, and so on, then you should obviously not pass them all through the constructor (at least, not one by one). One approach to handle this can be to introduce one or more service facades. Another approach might be to add some properties (for things which are optional or might be changed later, as described in @Stephen's answer).

To make decision which is the best solution, however, one has to look at the actual parameters and their meaning, you cannot make a good decision just on "number and types" of the parameters. I assume your "car" example is just a placeholder for something different in your real program, for which the situation might be different, but to discuss it in terms of a car: the owner's name is typically a property of the owner, not of the car, and the list of tickets is AFAIK something associated with the owner, too. So you might consider to introduce an Owner class for this example, where the owner has a name and a list of tickets. So this leads to a constructor

 public Car(Owner owner, IEngine engine)

However, taking Stephen's advice into account, assuming the owner of a car can change over the lifetime of a car object, it might be indeed better to stick to your original version 1, provide a getter and a setter for the "current owner" and implement your PrintOwnerName() function with regards to the fact that the owner of the car might be "unknown" (which means uninitialized).

4

If a property meets the following criteria then I make it a constructor parameter:

  • The class is dependent on it for operation
  • It has no reason to change over the lifetime of the object
  • Its value is known at object creation

If it meets these criteria, then it's a constructor parameter. If it only meets one or two of the criteria then it's almost certainly a settable property.

Constructor parameters are supposed to be used to inject the dependencies of the object. Use them for that and profit.

1

IMHO, it depends upon the parameters. I've used DI to inject cross cutting concerns, such as logging and session management. I think this is good use of DI as the magic of DI does not affect the class-specific logic.

Injecting instance specific parameters seems to be asking for trouble. Not the least of which is someone else trying to work out how your program works. Sometimes you write a bit of extra code to make the program simpler and easier to understand.

  • I agree with this - version 2 is more complex and wouldn't be able to be wired together by a framework such as spring. In my mind, once DI is too complex for a framework to do for you, it's too complex period. – Nathan Dec 8 '14 at 5:01
  • 2
    @Nathan: if a DI framework is not able to wire together such a simple thing as version 2, the DI framework is crap. Enforcing one to use property injection instead of ctor injection for simple parameters like the ones in this example does in no way improve the design. – Doc Brown Dec 8 '14 at 6:30
  • 1
    "Injecting instance specific parameters seems to be asking for trouble" - maybe I misunderstand what you are trying to say, but parameters in a constructor are always "instance specific" (otherwise they would not make much sense). Version 1 uses also an instance specific parameter. – Doc Brown Dec 8 '14 at 8:04
0

Version 1 is better than version 2. The reason for that is, a car should not know anything about an owner or any tickets. As cars are objects in our real world, think about it:

Can a car exist without having an owner or tickets? - Certainly yes.

Therefore, remove those from car and make car an independent class that is only dependent of what it is dependent in our world: an engine and possibly other technical properties.

Next, think about what determines the owner of a car. That would normally be a government-service that has a map to assign an owner to a car. Same goes for tickets. You can go even further and say that it does not even depend on that car itself but rather on the license that the government has given to the combination of the car and the owner.

But in your case a more simple modelation will be sufficient I guess, like having only one car-owner-service / car-ticket-service.

One of the many advantages of this modelation is for example, that you can give a college the car class so that he can use it anywhere he wants without having to know anything about tickets or having to deal with car owners. Furthermore, later you may come into a situation where you want a car to be able to have for example multiple owners, one for each country? Or maybe even more properties like a list of the ones allowed to drive it? In this case, you don't need to change the car class for that, but only the mentioned service. With your version 1 you are therefore violation the SRP.

In a nutshell: yes, version 2 is bad design. Go with the first version (and remove PrintOwnerName).

  • Since the OP is asking specifically for DI and the number of parameters of the ctor, we should probably assume in version 1 there is also an owner and a ticket list somewhere in the class car. Those parameters are just not passed through the constructor. So your answer is probably correct, but it does not answer the question. – Doc Brown Dec 8 '14 at 11:05
  • As my answer is too long to add it as a comment, what should I do in this situation? Just say nothing? – valenterry Dec 8 '14 at 11:17
  • Before posting an answer like this, you could ask the OP first in a comment for clarification if his original problem is really about car objects, or if the "car" class is just a (maybe badly chosen) placeholder for a different situation with "one ctor parameter" vs. "multiple parameters". – Doc Brown Dec 8 '14 at 12:17

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.