Is extracting an interface just for testing purposes a code smell?

Question

I will explain with an hypothetical example.

Suppose that my domain is Cars. Everyone around the software, talks about cars. Car is the aggregate root of aggregate roots. For example, CAR table has 150+ columns in database (irrelevant for this question, but for you to imagine).

So, there is class in the system called Car. This class has a lot of fields and behavior in it.

Suppose that given a Car, we have to calculate its HorsePower. This calculation has its own rules and logic. But for the calculation only a part (some fields) of the Car is required. And I want to extract this logic from the Car class in order to make it more visible where this happens, how it happens and what information is needed to happen.

So I end up with something like this, which is pure functional:

public class HorsePowerCalculator{
    HorsePower calculate(Car c){..}
}

My doubt is the following. If I pass a Car object to calculate, I have 2 issues (according to my opinion).

a) What HorsePowerCalculator needs in order to do its job, its not pretty straight forward. Because I pass the whole Car object, HorsePowerCalculator can access all (read) properties of a car.

b) For testing HorsePowerCalculator in the unit test level, I need to create a Car object which is not a trivial job. Other things must be taken to consideration where they are irrelevant for HorsePowerCalculation.

So, I was thinking that I solve the above mentioned issues by doing this:

public interface HasHorsePower{
    int getNumberOfPistons();
    EngineType getEngineType();
    FuelType getFuelType();

    //HorsePower needs also non-cohesive properties.
    //This is why I can't group the properties to i.e "Engine" class
    int getNumberOfWheels();  
}

Then:

public class Car implements HasHorsePower{

}

And finally:

public class HorsePowerCalculator{
    HorsePower calculate(HasHorsePower something){..}
}

After this, I gain:

a)it is pretty straight forward what information is needed to calculate horse power. All property/methods of the interface

b)Testing calculations and logic of HorsePowerCalculator means a TestDouble in the test suite with just setters/getters. Then just assert the calculation results.

My doubts are that the only HasHorsePower implementation will be...well.. only the Car

Is this solution a code smell, or let's say not optimal? What should I do in this case?

Answer 1

In the described context, there is some unstructured legacy code. Now to improve this situation, you add more structure to it by using classes and interfaces for creating sensible abstractions - just the same way like you do it by extracting functions or methods from other functions which have become too large over time.

Where I work, we would call this simply "cleaning up the code", or "basics of software design" - that's pretty much the opposite of a code smell. A "code smell" could be a class which has grown too large over time and motivates you to refactor parts out of it. This can lead to functions, classes and interfaces which are just used in only one place, there is nothing special about it.

Answer 2

The central misconception causing you to doubt this design is right on your question:

My doubts are that the only HasHorsePower implementation will be...well.. only the Car

If you introduce test fakes, mocks or doubles, then the interface has at least two implementations: Car and the test mock.

Avoid creating interfaces implemented by only a single class. This is an application of the YAGNI principal (You Ain't Gonna Need It). Introducing interfaces like this used to be justified in case you need more implementations in the future without a concrete plan to introduce them (hence "You Ain't Gonna Need It"). Many times interfaces were introduced without adding test mocks — and that is the key to understanding the advice.

As soon as you add test fakes, mocks, or stubs, you have more than one implementation. Right away, the advice to avoid introducing interfaces with a single implementation does not apply.

Testing non-trivial logic is desirable, and if defining an interface to facilitate testing achieves that goal, you have a valid design decision to do so. VoiceOfUnreason mentions that you get a feeling of "this should be easier to test", but I would like to take that feeling one step further. Not only should this class be easier to test, but writing tests for it allows you too think about the design of the Car class from a more abstract perspective. Writing tests can help you identify proper abstractions in existing code. When you cannot disassemble the large legacy class into multiple classes, defining an interface for a related subset of functions is a valid way to improve the design of new code, not just facilitate testing.

So to directly answer your question: yes it is perfectly acceptable to define an interface for testing purposes as long as that interface represents behavior that should be mocked, stubbed, or faked in your tests. Each mock, stub, or fake is a valid implementation of the interface, so you aren't creating an interface with only one implementation.

Answer 3

Is extracting an interface just for testing purposes a code smell?

No.

In fact, what you describe is one of the kinds of pressure that tests place on design -- the feeling that "testing this code should be easier than it is" is supposed to be motivation for changing your design until testing that code is as easy as it is supposed to be.

This is one of the important kinds of feedback that you get in test-first design/test driven development, where it is likely that tests motivate the refactoring of your code because the tests are one of the earliest clients that you have.

It looks to me that you are extracting a Role Interface - one of the things that a car can do is provide data to a horsepower calculation, via a particular contract.

For deeper analysis on the power of roles, look into the writings of Rebecca Wirfs-Brock and the topic of Responsibility Driven Design.

Answer 4

Considering your Car already exists in the database as a flat one-size-fits-all entity and you have to work with that, this seems a valid and useful approach. The only criticism I have is with the name of the interface which does not cover its use case very well. Something like IPowerProps seems more appropriate.

Answer 5

How do you know it will only be used for testing?

As Greg Burghardt points out, you are also using it for the production code — but that is not really the point.

The important thing is, does the interface segregation make sense?

What is the category of things of which you might reasonably ask “What is the power output of this?” What is the category of things on which your implementation of HorsePowerCalculator can actually work? Those two categories right now are identical (containing only Car and the test mock) — but it seems like they will track closely in the future. If the system adds Truck or Lawnmower, it will behave the same.

Finally, how confusing is this to the next person who reads the code (which will likely be you, in six months when you have forgotten the writing process and have been assigned to maintain “your” code)? It looks pretty sensible to me, but I don’t know your whole domain.

As a side-note, horsepower is just a unit. What you are calculating is power output. One HP is 745.7 watts. Formally, saying “calculating the horsepower” is like describing measuring the length of a board as “counting the centimeters”.

If your language and environment supports it, I would recommend you use something like Java’s measure and implement things like this as calculatePowerOutput() and return a Quantity of type Power in Horsepower units.

(I realize that your example might be wholly hypothetical, in which case take my advice in the abstract.)

Answer 6

I'd take a look at law of Demeter or principle of least knowledge and take it one step further. You can in this case refactor

public class HorsePowerCalculator{
    public HorsePower calculate(Car c){..}
}

to

public class HorsePowerCalculator{
    public HorsePower calculate(int numberOfPistons, EngineType engineType, FuelType fuelType){..}
}

and then you can test that unit in complete isolation from the Car, truck or bagel wagon which provides those values.

Once you need to deal with more parameters, you'd tend to wrap up the calculation in an object which encapsulates the data and behaviour together.

public record HorsePowerCalculator(int numberOfPistons, EngineType engineType, FuelType fuelType)
{
    public HorsePower calculate(){..}
}

Again, this creates no coupling with Car and so is easy to test, though it does require something in the domain model to know about both the entity and the calculator to apply the properties from the entity to the calculator.

Answer 7

Extending on Pete Kirkham's answer, this is how we solve this issue (pragmatically) in our applications:

public class HorsePowerCalculator
{
    public HorsePower calculate(Car car)
    {
        return calculate(car.getNumberOfPistons(), ...);
    }

    public HorsePower calculate(int numberOfPistons, EngineType engineType, FuelType fuelType)
    {
        ...
    }
}

In C#, we declare the second method as internal and use the InternalsVisibleTo attribute to make this method available (only) to the unit testing project. I don't know if Java offers a similar feature.

Yes, this violates the rule that unit tests should only test publicly available functionality, but it's the compromise that we found works best for us to both (a) unit-test specific algorithms and (b) keep the "regular" code short and easy to read.

Answer 8

1. If you have a large, flat object like "Car" that has a whole lot of aspects (like engine, gearbox, tires,...) then you have very probably designed that object too course-grained (exposing way too many details) in the first place.

   Your concerns are probably just a symptom of that and exposing an engine-only interface is only the second-best solution. The best method in my opinion would be to extract all of the engine-related stuff into a member-object engine owned by car that has all the engine-related stuff, and will, naturally, have a method calculateHorsepower.

   Separation of concerns is the keyword here.

2. As a programmer, as soon as I see an interface, I'm expecting to find multiple classes that implement it to expose a common interface to non-related classes (that's, after all, what interfaces are good for). Your code doesn't seem to have that, and I would spend valuable time searching for "whoever else implements this?". That defies the rule of "meet your successors' expectations" and makes your code less maintainable.

In case you really cannot extract-out the "horsepower-related" things into a separate member object (sometimes, it's really for lack of trying ;) ), I really don't see how a (misleading, in my opinion) interface would be in any way better than just a simple method.

Answer 9

I think if you were to consider how you would model this if you were starting from scratch you'd see a few ways around this.

In your case, a car does not have a measure of horsepower, an engine does, so you could have an IEngine interface:

IEngine {
    int getNumberOfPistons();
    EngineType getEngineType();
    FuelType getFuelType();
}

And then you would probably look at it and think that a is not an engine, it has an engine, so you could have a slightly dirty refactor like so, assuming that these properties already exist on your monolithic Car object.

Car {

    int getNumberOfPistons();
    EngineType getEngineType();
    FuelType getFuelType();

    public IEngine Engine => new Engine(getNumberOfPistons(), getEngineType(), getFuelType());
}

Your method to calculate horsepower could then take in an IEngine:

public class HorsePowerCalculator{
    HorsePower calculate(IEngine engine){..}
}

---

Or possibly better, your IEngine interface could have a method that calculates this for you, without the need for your public properites then

IEngine {
   double CalculateHorsepower();
}

class PetrolEngine {
    ctor(int numPistons, EngineType engineType, FuelType fuelType) {
       ... set private props
    }
    double CalculateHorsepower() {
    }
}

Which has the advantage of making it relatively easy to later introduce electric cars (without pistons or fuel) and incorporate them into your Car (or Van come to that). Your class (still dirty) then becomes:

Car {

    int getNumberOfPistons();
    EngineType getEngineType();
    FuelType getFuelType();

    public IEngine Engine => new PetrolEngine(getNumberOfPistons(), getEngineType(), getFuelType());
}

//This then reads a bit better altogether
new Car(...).Engine.CalculateHorsePower();

It's not perfect by any means, but it is a small step towards a better structure for you and the PetrolEngine object is now easier to unit test.