Uniezależnienie klasy od globalnych stałych

Pierwsze podejście

    public static class ConfigHelper
    {
        public const int ChannelsPerSerialPort = 8;
        public const int TotalNumberOfChannelsWithSequence = 2 * ChannelsPerSerialPort + 1;
    }

    public class DataCombiner
    {
        private readonly double[] _lastCombinedValue =  
            new double[ConfigHelper.TotalNumberOfChannelsWithSequence];

        private void SetOutputValues(double[] firstValues, double[] secondValues)
        {
            firstValues.CopyTo(_lastCombinedValue, 0);
            secondValues.CopyTo(_lastCombinedValue, ConfigHelper.ChannelsPerSerialPort);

            _lastCombinedValue[_lastCombinedValue.Length - 1] = firstValues[ConfigHelper.ChannelsPerSerialPort];
        }

        // More logic using more ConfigHelper.ChannelsPerSerialPort
    }

W klasie DataCombiner korzystamy w wielu miejscach ze stałych globalnych zdefiniowanych w klasie ConfigHelper. Gdy chcemy zobaczyć wszystkie miejsca, w których używamy ConfigHelper.ChannelsPerSerialPort, to znajdziemy mnóstwo odwołań i nie będzie się chciało przeglądać wszystkich. Wymieszanie z innymi klasami nie jest dobre.

Drugie podejście

    public class DataCombiner
    {
        private readonly int _sequenceIndex = ConfigHelper.ChannelsPerSerialPort;
        private readonly int _channelsCount = ConfigHelper.ChannelsPerSerialPort;
        private readonly double[] _lastCombinedValue = 
            new double[ConfigHelper.TotalNumberOfChannelsWithSequence];

        private void SetOutputValues(double[] firstValues, double[] secondValues)
        {
            firstValues.CopyTo(_lastCombinedValue, 0);
            secondValues.CopyTo(_lastCombinedValue, _channelsCount);

            _lastCombinedValue[_lastCombinedValue.Length - 1] = firstValues[_sequenceIndex];
        }

        // More logic that is using _channelsCount and _sequenceIndex
    }

Jest nieco lepiej, ponieważ korzystamy ze stałych klasy, które raz inicjalizujemy. Mają swoje nazwy opisujące co robią w danej klasie.

Trzecie podejście

    public class DataCombiner
    {
        private readonly int _sequenceIndex;
        private readonly int _channelsCount;
        private readonly double[] _lastCombinedValue;

        public DataCombiner(int channelsCount)
        {
            _channelsCount = channelsCount;
            _sequenceIndex = channelsCount;

            _lastCombinedValue = new double[2 * channelsCount + 1];
        }

        private void SetOutputValues(double[] firstValues, double[] secondValues)
        {
            firstValues.CopyTo(_lastCombinedValue, 0);
            secondValues.CopyTo(_lastCombinedValue, _channelsCount);

            _lastCombinedValue[_lastCombinedValue.Length - 1] = firstValues[_sequenceIndex];
        }

        // More logic that is using _channelsCount and _sequenceIndex
    }

    // Utworzenie instancji
    var combiner = new DataCombiner(ConfigHelper.ChannelsPerSerialPort);

Tutaj najwięcej się pozmieniało (chociaż zmieniliśmy tylko konstruktor). Z klasy wyrzucone zostały wszystkie stałe, już nawet nie potrzebujemy nic wiedzieć o otaczającym świecie. Rozmiary zostaną przekazane w konstruktorze. To jest oczywiście zgodne z SRP itd.

Tylko po co mi taka niezależność tej klasy?! Tej cegiełki przecież nigdzie nie re-użyję. Mając te stałe od razu na miejscu miałem pewność, że nie dam do konstruktora czegoś złego.

Najważniejszą rzeczą w tym przykładzie są testy jednostkowe! Poniżej dwa przykłady testów – jeden dla wariantu pierszego i drugiego, kolejny dla wariantu trzeciego.

Stare testy

Akurat taka jest dziedzina (ConfigHelper.ChannelsPerSerialPort = 8) i jest tutaj tyle danych. Mnie te liczby przerażają, nie jest to ani czytelne, ani utrzymywalne. Komentarz świadczy o tym, że w innym wypadku można by w ogóle się nie połapać o co chodzi w tych liczbach.

        [Test]
        public void GetCombinedDataWithTheSameSequences()
        {
            // Arrange
            var dataCombiner = new DataCombiner();
            var firstData = new[] { 1.0, 2, 3, 4, 5, 6, 7, 8, 100 };
            var firstDataRepeat = new[] { 1.0, 2, 3, 4, 5, 6, 7, 88, 101 }; // Last value differs
            var secondData = new[] { 1.0, 2, 3, 4, 5, 6, 7, 8, 101 };
            var expectedValues = new[] { 1.0, 2, 3, 4, 5, 6, 7, 88, 1, 2, 3, 4, 5, 6, 7, 8, 101 };

            // Act
            dataCombiner.PushFirst(firstData);
            dataCombiner.PushFirst(firstDataRepeat);
            
            // Assert
            Assert.IsTrue(dataCombiner.PushSecond(secondData));
            Assert.AreEqual(expectedValues, dataCombiner.CombinedValue());
        }

Nowe testy

Można to zrobić zdecydowanie prościej i czytelniej. Takie testy to aż chce się pisać. I właśnie o tego typu usprawienia chodzi w tym poście.

        [Test]
        public void GetCombinedDataWithTheSameSequences()
        {
            // Arrange
            var dataCombiner = new DataCombiner(1);
            var firstData = new[] { 1.0, 100 };
            var firstDataRepeat = new[] { 2.0, 101 };
            var secondData = new[] { 3.0, 101 };
            var expectedValue = 2.0;

            // Act
            dataCombiner.PushFirst(firstData);
            dataCombiner.PushFirst(firstDataRepeat);
            
            // Assert
            Assert.IsTrue(dataCombiner.PushSecond(secondData));
            Assert.AreEqual(expectedValue, dataCombiner.CombinedValue()[0]);
        }

PS Warto też dodać jeden test sprawdzający (jeden wystarczy), czy klasa radzi sobie z większymi danymi (new DataCombiner(8))

PS2 Wie ktoś jak sprawić, żeby klasa ConfigHelper była jakimś innym kolorem zaznaczona i nie zlewała się tak z pozostałym kodem?

Reklamy
Ten wpis został opublikowany w kategorii Lessons Learned i oznaczony tagami , . Dodaj zakładkę do bezpośredniego odnośnika.