patterncsharpModerate
Car presenter tests
Viewed 0 times
presentercartests
Problem
I caught a whiff of a code-smell emanating from one of my tests, in a scenario akin to the following:
I was struggling to describe what the problem is except that it seems wrong that a
I wondered what pointers people here might give me?
[TestFixture]
public void CarPresenterTests{
[Test]
public void Throws_If_Cars_Wheels_Collection_Is_Null(){
IEnumerable wheels = null;
var car = new Car(wheels);
Assert.That(
()=>new CarPresenter(car),
Throws.InstanceOf()
.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?
Solution
Is a car really a car without wheels?
If not, the check should be done at
Assuming of course that
And while we are talking about smells... You do not have much encapsulation.
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
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.Context
StackExchange Code Review Q#3054, answer score: 10
Revisions (0)
No revisions yet.