Car presenter tests

Question

I caught a whiff of a code-smell emanating from one of my tests, in a scenario akin to the following:

[TestFixture]
public void CarPresenterTests{

    [Test]
    public void Throws_If_Cars_Wheels_Collection_Is_Null(){
         IEnumerable<Wheels> wheels = null;
         var car = new Car(wheels);
         Assert.That(
         ()=>new CarPresenter(car),
         Throws.InstanceOf<ArgumentException>()
         .With.Message.EqualTo("Can't create if cars wheels is null"));
    }
}

public class CarPresenter{
    public CarPresenter(Car car)
    {
        if(car.Wheels == null)
            throw new ArgumentException("Can't create if cars wheels is null");
        _car = car;
        foreach(var wheel in _car.Wheels)
        {
            wheel.Rolling += WheelRollingHandler;
        }
    }
}

I was struggling to describe what the problem is except that it seems wrong that a CarPresenter should attempt to dictate to a Car whether or not its Wheels are initialised correctly.

I wondered what pointers people here might give me?

Answer 1

Is a car really a car without wheels?

If not, the check should be done at Car construction time and CarPresenter should not have to check for null, it should assume that a good working car is passed to it. (Correction, CarPresenter should check at construction time that Car is not null.)

Assuming of course that Car is a class that you control as well therefore you can change it.

And while we are talking about smells... You do not have much encapsulation. Wheels on Car should probably be private with public accessor methods if needed.

Answer to comment:

So would you just handle the event Wheels.Rolling event and accept that if Wheels is null an exception will be thrown

No, if you check at construction time and make sure you have working wheels, then you do not need to worry about it later. It is a lot better to bluntly (runtime exception) point out as early as possible if someone made a mistake (passing null for wheels) than waiting for these wheels to blow up who knows where in the code.

Answer 2

First off - I do not like the idea of asserting on an exception message. I'd rather have the class throw a WheelNotFoundException rather than an ArgumentException. This could be easily asserted on.

Secondly, the smell I see are from the class itself:

1. As @c_maker pointed out, the check for existence of wheels should be done while creating a Car. It is not the CarPresenter's responsibility to check if the car is valid. Besides, the CarPresenter takes a Car as a parameter - this contract itself should tell the CarPresenter that the Car is properly created.

2. Again a bit of violation of encapsulation - CarPresenter should deal with just the Car object, and not access its member's properties directly. In this case I'd rather have a Car.OnMove event handled at CarPresenter and in Car class should chain the event to the Wheel's rolling.

3. Car has Wheels which is a collection of Wheel. Are you looking at a generic event handler for the Wheels collection? I would prefer each Wheel having its own event.

Answer 3

Basically, by passing wheels in the constructor, you are saying its public interface has something to do with giving it wheels. Meaning anything that makes cars might give the wrong input.

Encapsulate building of cars:

builder = new CarBuilder();

var car = builder.Car().Wheel().Wheel().Wheel().Wheel().SteeringWheel().AsCar();

You could even make wheel factories and such for different types of wheels.