0

Do you think its a good OO practice to use parts of an object to build itself? Here is an example.

var car = new Car 
    {
        Make  = BuildMake(),
        Model = BuildModel(),
        Vin   = BuildVin(),
    };



car.Wheel = BuildWheel(Car.Vin);

In the case above, I had to move the wheel out of the initialize because I need the data from Vin to get it. Is there anything bad with this approach? or would a better approach be

var car = new Car 
    {
        Make  = BuildMake(),
        Model = BuildModel(),
        Vin   = BuildVin(),
        Wheel = BuildWheel(BuildVin())
    };

But in the case above, we will have to call BuildVin() twice which means we are duplicating effort. From a good object oriented design perspective, what approach would one recommend?

  • 1
    In the second case, why not plug Vin into BuildWheel instead of calling BuildVin again? Are those BuildXXX functions not called in the order listed? – John Douma Mar 6 at 0:38
  • Are these code snippets executing from within a static method of the Care class? – Greg Burghardt Mar 6 at 13:36
  • I think I would use a factory to build my cars. en.wikipedia.org/wiki/Factory_method_pattern – gam3 Mar 6 at 15:46
3

Assign the built Vin to a local variable. Use that variable to set the Car's Vin-propert and to build the Wheel:

var vin = BuildVin();
var car = new Car
{
    Make = BuildMake(),
    Model = BuildModel(),
    Vin = vin,
    Wheel = BuildWheel(vin)
};

This allows initialising all properties at once and not having incomplete Car-objects. It requires Vin to be usable (or at least intiantable) outside of Car.

Separate building a Car from using a Car using the builder pattern (when it makes sense, the builder's methods can set multiple properties at once):

var carBuilder = new CarBuilder();
carBuilder.SetMakeAndModel("Ford", "T");
carBuilder.SetWheelsWithVins();

Car car = carBuilder.Build();

SetMakeAndModel() can check that you are trying to build a valid combination. SetWheelsWithVins() solves the problem of needing the Vin to set Wheel. In c#, the combined methods can be extension methods separate from the CarBuilder-class. And since carBuilder is supposed to be incomplete, using your first approach of setting Wheel after initialising carBuilder is not a problem at all.

2

Answer

To directly answer your question, you should just use temporary variables:

Make make = BuildMake(),
Model model = BuildModel(),
Vin vin = BuildVin(),
Wheel wheel = BuildWheel(vin);

var car = new Car 
{ 
    Make = make,
    Model = model,
    Vin = vin,
    Wheel = wheel
};

Taking things apart like this also makes it easier to modify it later. If some changes later make it necessary to use 2 additional steps to create the wheel from the VIN, you can just slip them in between there.

As a side note

From the context you are giving here, I would assume the rule "Every Car has a Make, Model, Vin and Wheel" - but using the object initializer as you do, you could also write

var car = new Car 
{ 
    Make = make,
    Model = model
};

Wheel missing, VIN missing, and nothing is telling you that you forgot something.

Constructors

If indeed every Car is supposed to have Make, Model, Vin and Wheel, then you should encapsulate that rule in code, by using a constructor like this:

public class Car
{
    public Car(Make make, Model model, Vin vin, Wheel wheel)
    {
        Make = make;
        Model = model;
        Vin = vin;
        Wheel = Wheel;
    }

    public Make Make { get; set; }
    public Model Model { get; set; }
    public Vin Vin { get; set; }
    public Wheel Wheel { get; set; }
}

That way, your code is communicating "This class needs these things". For creating it, instead of

var car = new Car 
{ 
    Make = make,
    Model = model,
    Vin = vin,
    Wheel = wheel
};

You'll use

var car = new Car ( make, model, vin, wheel );
1

This is a little nuanced as construction always involves objects in an in-determinant state. I think it is preferable to minimise the amount of work that's done in this state. However, I think if you are resigned to this, the first is better. BuildVin() is called only once, and the state of construction is known even if not complete.

  • Objects may necessarily be in indeterminate state within the constructor, but for good practice, objects should always hold all its invariants the moment it returns from its constructor. It's usually bad practice to require the caller to call a number of other methods before the class gets into its proper state. – Lie Ryan Mar 6 at 11:42
1

With the second approach, you not only waste CPU resources by calling BuildVin() twice, you might also end up with a different, unexpected result.

If BuildVin() creates a fresh VIN instance (which the name implies), then you'll end up with one VIN instance stored in the Vin field, and a different one used to create the wheel, which might lead to subtle, nasty problems, e.g. if you change something inside the Vin object, but don't see that reflected at the VIN that's used inside the Wheel.

1

As you think, do not use second one. There are many reason beyond duplicating effort. Also there is business responsible. Every changes, you need to implement to more than one place.

When considering your first approach, it is important what your aim is. If you want to prepare car instance to ignore null reference and this is what you are asking for, then you can initialize all property in Car class constructor.

public class Car
{
    public Car()
    {
        Make = BuildMake();
        Model = BuildModel();
        Vin = BuildVin();
        Wheel = BuildWheel(Vin);
    }

    public Make Make { get; set; }
    public Model Model { get; set; }
    public Vin Vin { get; set; }
    public Wheel Wheel { get; set; }
}

But if you want to initialize those properties by different function per instance like :

var car = new Car 
{
    Make  = BuildMake(),
    Model = BuildModel(),
    Vin   = BuildVin(),
};

var car2 = new Car 
{
    Make  = BuildMake2(),
    Model = BuildModel2(),
    Vin   = BuildVin2(),
};

Then you can create new classes Ford and Toyota and do your initialize in your new classes.

public class Ford : Car
{
    public Ford()
    {
        Make = BuildMake();
        Model = BuildModel();
        Vin = BuildVin();
        Wheel = BuildWheel(Vin);
    }
}

public class Toyota : Car
{
    public Toyota()
    {
        Make = BuildMake2();
        Model = BuildModel2();
        Vin = BuildVin2();
        Wheel = BuildWheel2(Vin);
    }
}

Then, initialize those instance:

var ford = new Ford(); 
var toyota = new Toyota();

This is proper OO practice I think.

Note: If you use Ford and Toyota classes, then it is better to change Car as abstract class.

0

I like this approach more: Like Kasper van den Berg said.

var vin = BuildVin();
var car = new Car
{
    Make = BuildMake(),
    Model = BuildModel(),
    Vin = vin,
    Wheel = BuildWheel(vin)
};

One could also create a variable bin before creating Car

var vin = BuildVin();
var wheel = BuildWheel(vin);
var car = new Car
{
    Make = BuildMake(),
    Model = BuildModel(),
    Vin = vin,
    Wheel = wheel
};

I see nothing wrong with this per se.

0

Adding another option, you can create two classes for Wheel and InstalledWheel:

var car = new Car
{
    make = BuildMake(),
    model = BuildModel(),
    vin = BuildVin(),
    wheel = BuildWheel()
};

inside the Car constructor, you then do

class Car {
    ...
    InstalledWheel wheel;

    public Car(..., Vin vin, Wheel wheel) {
        this.wheel = wheel.Install(vin);
    }
}

Unless you want to allow the possibility of installing a Wheel with a non-matching vin (and where this is a useful configuration), your caller shouldn't have to care how to assemble the car together. If the caller have the components, it should be able to just pass the components without having to care how to set up the internal interdependency that it doesn't really need to care about.

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.