Is this the correct way to use and test a class that makes use of the factory pattern?

Question

I don't have a lot of experience with the factory pattern and I've come across a scenario where I believe it is necessary but I'm not sure the I've implemented the pattern correctly and I'm concerned about the impact it's had on the readability of my unit tests.

I've created a code snippet that approximates (from memory) the essence of the scenario I am working on at work. I'd really appreciate it if someone could take a look at it and see if what I've done seems reasonable.

This is the class I need to test:

public class SomeCalculator : ICalculateSomething
{
    private readonly IReducerFactory reducerFactory;
    private IReducer reducer;

    public SomeCalculator(IReducerFactory reducerFactory)
    {
        this.reducerFactory = reducerFactory;
    }

    public SomeCalculator() : this(new ReducerFactory()){}

    public decimal Calculate(SomeObject so)
    {   
        reducer = reducerFactory.Create(so.CalculationMethod);

        decimal calculatedAmount = so.Amount * so.Amount;

        return reducer.Reduce(so, calculatedAmount);
    }
}

Here are some of the basic interface definitions...

public interface ICalculateSomething
{
    decimal Calculate(SomeObject so);
}

public interface IReducerFactory
{
    IReducer Create(CalculationMethod cm);
}

public interface IReducer
{
    decimal Reduce(SomeObject so, decimal amount);
}

This is the factory I've created. My current requirements have had me add a specific Reducer MethodAReducer to be used in a particular scenario which is why I'm trying to introduce a factory.

public class ReducerFactory : IReducerFactory
{
    public IReducer Create(CalculationMethod cm)
    {
        switch(cm.Method)
        {
            case CalculationMethod.MethodA:
                return new MethodAReducer();
                break;
            default:
                return DefaultMethodReducer();
                break;
        }
    }
}

These are approximations of the two implementations... The essence of the implementation is that it only reduces the amount if the object is in a particular state.

public class MethodAReducer : IReducer
{
    public decimal Reduce(SomeObject so, decimal amount)
    {   
        if(so.isReductionApplicable())
        {
            return so.Amount-5;
        }
        return amount;
    }
}

public class DefaultMethodReducer : IReducer
{
    public decimal Reduce(SomeObject so, decimal amount)
    {
        if(so.isReductionApplicable())
        {
            return so.Amount--;
        }
        return amount;
    }
}

This is the test fixture I am using. What has concerned me is how much space in the tests the factory pattern has taken up and how it appears to reduce the readability of the test. Please keep in mind that in my real world class I have several dependencies that I need to mock out which means that the tests here are several lines shorter than the ones needed for my real world test.

[TestFixture]
public class SomeCalculatorTests
{
    private Mock<IReducerFactory> reducerFactory;
    private SomeCalculator someCalculator;

    [Setup]
    public void Setup()
    {
        reducerFactory = new Mock<IReducerFactory>();
        someCalculator = new SomeCalculator(reducerFactory.Object);     
    }

    [Teardown]
    public void Teardown(){}

First test

    //verify that we can calculate an amount
    [Test]
    public void Calculate_CalculateTheAmount_ReturnsTheAmount()
    {
        decimal amount = 10;
        decimal expectedAmount = 100;
        SomeObject so = new SomeObjectBuilder()
         .WithCalculationMethod(new CalculationMethodBuilder())                                                          
                     .WithAmount(amount);

        Mock<IReducer> reducer = new Mock<IReducer>();

        reducer
            .Setup(p => p.Reduce(so, expectedAmount))
            .Returns(expectedAmount);

        reducerFactory
            .Setup(p => p.Create(It.IsAny<CalculationMethod>))
            .Returns(reducer);

        decimal actualAmount = someCalculator.Calculate(so);

        Assert.That(actualAmount, Is.EqualTo(expectedAmount));
    }

Second test

    //Verify that we make the call to reduce the calculated amount
    [Test]
    public void Calculate_CalculateTheAmount_ReducesTheAmount()
    {
        decimal amount = 10;
        decimal expectedAmount = 100;
        SomeObject so = new SomeObjectBuilder()
         .WithCalculationMethod(new CalculationMethodBuilder())                                                          
                     .WithAmount(amount);

        Mock<IReducer> reducer = new Mock<IReducer>();

        reducer
            .Setup(p => p.Reduce(so, expectedAmount))
            .Returns(expectedAmount);

        reducerFactory
            .Setup(p => p.Create(It.IsAny<CalculationMethod>))
            .Returns(reducer);

        decimal actualAmount = someCalculator.Calculate(so);

        reducer.Verify(p => p.Reduce(so, expectedAmount), Times.Once());            
    }
}

So does all of that look right? Or is there a better way to use the factory pattern?

Mark Seemann Mark Seemann · Accepted Answer · 2009-12-12T07:11:34

It's a pretty long question you are asking, but here are some stray thoughts:

AFAIK, there's no 'Factory' pattern. There's a pattern called Abstract Factory and another one called Factory Method. Right now you seem to be using Abstract Factory.
There's no reason that SomeCalculator have both a reducerFactory and a reducer field. Get rid of one of them - in your current implementation, you don't need the reducer field.
Make the injected dependency (reducerFactory) readonly.
Get rid of the default constructor.
The switch statement in ReducerFactory may be a code smell. Perhaps you could move the creation method to the CalculationMethod class. That would essentially change the Abstract Factory to a Factory Method.

In any case, there's always an overhead associated with introducing loose coupling, but don't think that you are doing this for testability only. Testability is really only the Open/Closed Principle, so you are making your code more flexible in many more way than just to enable testing.

Yes, there's a small price to be paid for that, but it's well worth it.

In most cases, the injected dependency should be read-only. While not technically necessary, it is a good extra level of safety to mark the field with the C# readonly keyword.

When you decide to use DI, you must use it consistently. This means that overloaded constructors are yet another anti-pattern. This makes the constructor ambiguous and may also leads to Tight Coupling and Leaky Abstractions.

This cascades and may seem like a drawback, but is actually an advantage. When you need to create a new instance of SomeCalculator in some other class, you must again either inject it or inject an Abstract Factory that can create it. The advantage comes when you then extract an interface from SomeCalculator (say, ISomeCalculator) and inject that instead. You have now effectively decoupled the client of SomeCalculator from IReducer and IReducerFactory.

You don't need a DI Container to do all this - you can wire up instances manually instead. This is called Pure DI.

When it comes to moving the logic in ReducerFactory to CalculationMethod, I was thinking about a virtual method. Something like this:

public virtual IReducer CreateReducer()
{
    return new DefaultMethodReducer();
}

For special CalculationMethods, you can then override the CreateReducer method and return a different reducer:

public override IReducer CreateReducer()
{
    return new MethodAReducer();
}

Whether this last advice makes sense depends on a lot of information that I don't have, so I'm just saying that you should consider it - it may not make sense in your specific case.

Is this the correct way to use and test a class that makes use of the factory pattern?

1 Answers