31

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?

9
  • 34
    That's not just for testing, you're also achieving interface segregation - as you say, it makes it clear what aspects of a car (or other ICE-using device, even if you don't have any others currently) are relevant to calculating its horsepower, and isolates that calculation from the irrelevant aspects.
    – jonrsharpe
    Mar 19 at 8:37
  • 37
    It might be worth noting that “just for testing purposes” is a perfectly legitimate purpose. Software doesn't just have to work, it should also be verifiable whether it works. Thus, designing for testability is extremely useful. Consider your testing code and other dependent code to be of equal importance.
    – amon
    Mar 19 at 10:06
  • 9
    @GeorgeZ. That advice is mostly used to guard against overeager programmers who are new to OOP, look at their classes, and then create an interface with exactly the same methods as the class since “you should depend on abstractions, not on details”. In contrast, it is a very useful approach to use interfaces to describe actual requirements on a type. Not “I need a Car” but “I need the number of pistons”. Using interfaces or Parameter Objects to describe these requirements is fantastic, decouples code, is easy to test, and works well with the “programming by wishful thinking” technique.
    – amon
    Mar 19 at 10:31
  • 10
    To "Is doing X just for the sake of testing okay?", the answer is always "Yes". Your test suite is as much part of the code base as anything else, otherwise why bother having one? The fact that you don't ship it to the customer is irrelevant. Mar 19 at 10:48
  • 3
    'the idea of "dont create interface if you are going to have only one implementation" did it controversial for me'. This is a rule of thumb meant to prevent you from building in wrong abstractions prematurely (building into your code a structure that later on turns out to be too rigid), but if you want to chip away slowly at a legacy codebase, you need some way to introduce a separation (what Michael Feathers calls a seam) between the legacy code, and the reworked code, and an interface is a way to do that. And it may be an intermediate step - you may end up refactoring it away by the end. Mar 19 at 14:48

9 Answers 9

44

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.

5
  • 4
    unstructured legacy code describes exactly what I have. Thank you for answer. I will wait for some more feedback.
    – George Z.
    Mar 19 at 9:01
  • 1
    and HasHorsePower doesn't smell like a sensible abstraction to me!
    – user253751
    Mar 22 at 15:00
  • An important thing to note about code smells is that while they tend to indicate that there is likely to be a design defect somewhere, in many cases the defect may lie somewhere other than the "smelly" code. Especially if one is refactoring legacy systems, it's common for the refactoring to introduce code smells, because quirks in some parts of the code that cannot be readily changed make it necessary for other parts of the code to work around them in smelly fashion. If a design has some potentially problematic quirks that cannot be fixed, leaving latent code smells may be better...
    – supercat
    Mar 22 at 16:54
  • ...than trying to eliminate the smell without fixing the underlying quirks. Eliminating the quirks would be better yet, when that is possible, but leaving clear and obvious hazards is better than leaving hidden ones.
    – supercat
    Mar 22 at 16:55
  • I call our attention to Refactoring: Improving the Design of Existing Code. Then this excellent related talk. You don't know that you don't know code smell until you watch it. Examples are Ruby but Sandy Metz is a superior teacher. All of her talks are worth watching irrespective your particular OO programming language.
    – radarbob
    May 6 at 3:18
26

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.

1
  • I'd say an additional benefit of the only consumers being the production code and the test suite is it gives you freedom to freely and safely change the interface to best benefit the tests. You'd need to document that, obviously!
    – Ed.
    Mar 23 at 15:34
14

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.

3
  • 2
    BTW: horses have horsepower -- your interface here looks to be a collection of properties we expect to be implemented by an Engine. Mar 19 at 12:26
  • Thanks for the references. Really interesting! About interface properties and Engine etc, I know it! I just used these for the sake of my example. Thanks for the feedback.
    – George Z.
    Mar 19 at 13:32
  • '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.' -- I keep re-reading this answer, and I think this is the part that always jumps out. Very well said. It really drives to the heart of how unit testing and design play well together. May 11 at 12:28
5

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.

1
  • 1
    Agreed. I did not know what to put for the question. HorsePowerCalculatable sounds better :) This is something I will figure out later.
    – George Z.
    Mar 19 at 9:18
4

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.)

5
  • 1
    my "HorsePower" result contains many properties and has a lot of semantics in my domain. Plus, it is a core topic of the system. Thanks (for the feedback in general) for the Java's measure, but It does not match my needs.
    – George Z.
    Mar 19 at 18:53
  • This. Extracting interfaces isn't a code smell. The "just for testing" part is the code smell. Make sure you're thinking about your design. It seems a bit contradictory, but although code should be testable, tests shouldn't drive its design. Mar 22 at 17:41
  • "tests shouldn't drive its design" -- @TechInquisitor, this is precisely what Test-Driven Development advocates: tests should drive the design (not shouldn't). May 5 at 15:10
  • @GregBurghardt Testability should drive design whether you're practicing strict TDD or not. But the intended API still drives the design of the tests, not the other way around. For example, never break encapsulation just for the purpose of testing. May 5 at 15:57
  • That is true, but it didn't appear this question was about breaking encapsulation for testing purposes. May 5 at 16:12
2

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.

2

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.

1
  • Maybe you've already considered this, but with C# you can also use reflection to stub out the car. Mar 22 at 12:52
1
  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.

10
  • 1
    Of course I tried this. Let say in my real world, the HorsePowerCalculation depends also in "Tires" and "Windows". There is no an "Engine" where I can put it that will make the perfect sense.
    – George Z.
    Mar 19 at 14:42
  • @GeorgeZ. if it requires that much to calculate horsepower, maybe that calculation should be a concern for Car, and no additional abstraction exists. This becomes challenging when that calculation reaches out of the current process to an outside resource in order to crunch numbers. Mar 19 at 17:43
  • How much horsepower an engine can provide is independent of how much horsepower a car may need due to its tires and windows. Which are you calculating?
    – chepner
    Mar 19 at 18:43
  • 1
    "I want to pass to "HorsePowerCalculator" an immutable "input object" and return an "immutable output object". Then just copy the results to car." - That's, to a large extent, the definition of a member function. Don't understand why you would use an interface.
    – tofro
    Mar 21 at 17:05
  • 2
    "As a programmer, as soon as I see an interface, I'm expecting to find multiple classes that implement it". Not me, as a programmer, I'm expecting to find code that depends on these classes to be isolated and tested independently, which should make the code more maintainable.
    – StuperUser
    Mar 22 at 9:36
1

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.

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.