Tuesday 28 May 2013

Don't write new unit tests during the refactoring phase

If you're doing unit tests, you will be familiar with the Red-Green-Refactor cycle - write a failing test (red), make it pass (green) then drive out the design of the code (refactor).

When it comes to the refactor phase, you shouldn't need to write new tests - the refactor phase is when you change the implementation of the code, not the behaviour.

Take the following green code, which returns the min and max temperatures for three states of water:
using System;
namespace H2OLib
{
    public class H2O
    {
        public enum State
        {
            Gas,
            Liquid,
            Solid
        }

        private readonly State _state;

        public H2O(State state)
        {
            if (state != State.Gas && state != State.Liquid && state != State.Solid)
            {
                throw new ArgumentOutOfRangeException();
            }
            _state = state;
        }

        public int MaxTemp
        {
            get
            {
                switch (_state)
                {
                    case State.Gas:
                        return 374;
                    case State.Liquid:
                        return 100;
                    default:
                        return 0;
                }
            }
        }

        public int MinTemp
        {
            get
            {
                switch (_state)
                {
                    case State.Gas:
                        return 100;
                    case State.Liquid:
                        return 0;
                    default:
                        return -230;
                }
            }
        }
    }
}
This code is tested by the following tests:
    [TestFixture]
    public class H2OTests
    {
        [Test]
        public void GivenGasStateThenExpectCorrectTemps()
        {
            var h2o = new H2O(H2O.State.Gas);
            Assert.That(h2o.MinTemp, Is.EqualTo(100));
            Assert.That(h2o.MaxTemp, Is.EqualTo(374));
        }

        [Test]
        public void GivenLiquidStateThenExpectCorrectTemps()
        {
            var h2o = new H2O(H2O.State.Liquid);
            Assert.That(h2o.MinTemp, Is.EqualTo(0));
            Assert.That(h2o.MaxTemp, Is.EqualTo(100));
        }

        [Test]
        public void GivenSolidStateThenExpectCorrectTemps()
        {
            var h2o = new H2O(H2O.State.Solid);
            Assert.That(h2o.MinTemp, Is.EqualTo(-230));
            Assert.That(h2o.MaxTemp, Is.EqualTo(0));
        }

        [Test]
        public void GivenUnknownStateThenExceptionThrown()
        {
            Assert.Throws<ArgumentOutOfRangeException>(() => new H2O((H2O.State)99));
        }
    }

The code behaves exactly as planned, but the switch statements are a bit smelly. One solution is to refactor to the State pattern, with a factory to instantiate the correct state class based on the value that is passed to the H2O constructor. So, what's the next thing you do? Is it a) write a test for the StateFactory:
namespace H2OLib.Test
{
    [TestFixture]
    public class StateFactoryTests
    {
        [Test]
        public void ConstructExpectNoError()
        {
            Assert.DoesNotThrow(() => new StateFactory());
        }
    }
}
or b) don't write a test for the StateFactory:
// This line left intentionally blank
If you read the title of the post, you'll probably know that the answer is b. Given the tests that are in place, the code can be refactored to the following with no additional tests required:
using System;

namespace H2OLib
{
    public class H2O
    {
        private static StateFactory _stateFactory = new StateFactory();

        public enum State
        {
            Gas,
            Liquid,
            Solid
        }

        private readonly IState _state;

        public H2O(State state)
        {
            _state = _stateFactory.CreateState(state);
        }

        public int MaxTemp
        {
            get
            {
                return _state.MaxTemp;
            }
        }

        public int MinTemp
        {
            get
            {
                return _state.MinTemp;
            }
        }
    }

    public interface IState
    {
        int MinTemp { get; }
        int MaxTemp { get; }
    }

    public class StateFactory
    {
        public IState CreateState(H2OLib.H2O.State state)
        {
            switch (state)
            {
                case H2O.State.Gas:
                    return new GasState();

                case H2O.State.Liquid:
                    return new LiquidState();

                case H2O.State.Solid:
                    return new SolidState();

                default:
                    throw new ArgumentOutOfRangeException();
            }
        }
    }

    public class GasState : IState
    {
        public int MinTemp
        {
            get { return 100; }
        }

        public int MaxTemp
        {
            get { return 374; }
        }
    }

    public class LiquidState : IState
    {
        public int MinTemp
        {
            get { return 0; }
        }

        public int MaxTemp
        {
            get { return 100; }
        }
    }

    public class SolidState : IState
    {
        public int MinTemp
        {
            get { return -230; }
        }

        public int MaxTemp
        {
            get { return 0; }
        }
    }
}
When you write a new test you're not refactoring; it's a new red phase.

No comments:

Post a Comment