0

Refactoring – początek serii

Cześć! Ostatnio tj. po opublikowaniu zadania z serii DailyChallenge tj. zadania nr #38, dostałem opierdziel, że kod jest – delikatnie mówiąc – słaby. Jak najbardziej się z tym zgadzam i krytyka była zasadna. Jednak seria DailyChallenge nie polegała na pisaniu kodu wysokiej jakości, a bardziej szybkim rozwiązaniu danego problemu.

Nie zmienia to faktu, że codziennie w pracy staram się pilnować jakości kodu zarówno samemu go pisząc, jak i sprawdzając CR, a tutaj, na blogu wrzucam takie coś.

Nie zmienię sposobu publikacji zadań z seria DailyChallenge, ale zamiast tego postanowiłem rozpocząć nową serię artykułów pt. Refactoring.

Cel artykułów Refactoring

W artykułach z cyklu Refactoring, będę brał jakiś kod z języka C# i dokonywał jego refaktoryzacji. Pochodzenie kodu nie jest bliżej określone, ale mam duży zbiór starych projektów, które napisałem wiele lat temu, więc może to być idealne źródło słabej jakości kodu.

W każdym artykule poruszę tylko jeden większy lub mniejszy (raczej mniejszy, aby artykuł nie zajął kilku stron) kawałek kodu, omówię go, wyznaczę słabe punkty i krok po kroku będę go poprawiał.

Na samym końcu podsumuję wszystko co zostało zrobione w kodzie, wymienię zasady, którymi się kierowałem i wzorce projektowe, których użyłem. Postaram się to robić na tyle jasno, żeby osoby, które dopiero będą się uczyć pisać czysty kod, wszystko zrozumiały.

Artykuł w serii będę publikować w każdy poniedziałek, tak aby dobrze zacząć tydzień :D.

Przy okazji tematu refactoringu, polecam książkę Wujka Boba, którą opisałem w czytelni.

Refactoring #0 – Problem

Po tym nieco przydługim wstępie, przejdźmy do rzeczy. Co bardziej nadaje się na dziewiczy wpis niż kod, który podyktował powstanie serii? Nic. Tak więc, na ramach tego wpisu dokonam refaktoryzacji kodu z zadania DailyChallenge #38.

Kod prezentuje się następująco:

public class DailyChallenge38
{
    public class Field : IEquatable<Field>, IComparable<Field>
    {
        public int X;
        public int Y;
        public bool Live;
        public bool NextLive;
        public Field N;
        public Field W;
        public Field NW;
        public Field NE;
        public Field E;
        public Field S;
        public Field SE;
        public Field SW;
 
        public int CompareTo(Field other)
        {
            return X.CompareTo(other.X);
        }
 
        public bool Equals(Field other)
        {
            return other?.X == X && other.Y == Y;
        }
    }
 
    private Dictionary<int, List<Field>> _map = new Dictionary<int, List<Field>>();
 
    public DailyChallenge38(IEnumerable<Tuple<int, int>> lives)
    {
        foreach (var live in lives)
        {
            if (_map.ContainsKey(live.Item2) == false)
            {
                _map[live.Item2] = new List<Field>();
            }
            Field field = new Field { X = live.Item1, Y = live.Item2, Live = true, NextLive = true };
            if (_map[live.Item2].Contains(field))
            {
                continue;
            }
            _map[live.Item2].Add(field);
        }
        SortMap();
    }
 
    private void SortMap()
    {
        foreach (var set in _map.Values)
        {
            set.Sort();
        }
    }
 
    public void Proceed(int n)
    {
        if (n < 0)
        {
            throw new ArgumentException();
        }
        Print();
        for (int i = 0; i < n; ++i)
        {
            AddNew();
            MakeStep();
            Print();
        }
    }
 
    private void AddNew()
    {
        // foreach add neighbours
        foreach (var pair in _map.ToList())
        {
            foreach (Field field in pair.Value.ToList())
            {
                if (field.N == null)
                {
                    field.N = AddNeighbour(field.X, field.Y - 1);
                    field.N.S = field;
                }
                if (field.S == null)
                {
                    field.S = AddNeighbour(field.X, field.Y + 1);
                    field.S.N = field;
                }
                if (field.E == null)
                {
                    field.E = AddNeighbour(field.X + 1, field.Y);
                    field.E.W = field;
                }
                if (field.W == null)
                {
                    field.W = AddNeighbour(field.X - 1, field.Y);
                    field.W.E = field;
                }
                if (field.NE == null)
                {
                    field.NE = AddNeighbour(field.X + 1, field.Y - 1);
                    field.NE.SW = field;
                }
                if (field.SE == null)
                {
                    field.SE = AddNeighbour(field.X + 1, field.Y + 1);
                    field.SE.NW = field;
                }
                if (field.NW == null)
                {
                    field.NW = AddNeighbour(field.X - 1, field.Y - 1);
                    field.NW.SE = field;
                }
                if (field.SW == null)
                {
                    field.SW = AddNeighbour(field.X - 1, field.Y + 1);
                    field.SW.NE = field;
                }
            }
        }
        SortMap();
    }
 
    private void MakeStep()
    {
        foreach (var pair in _map)
        {
            foreach (Field f in pair.Value)
            {
                int livingNeigh = 0;
                livingNeigh += IsLive(f.N) ? 1 : 0;
                livingNeigh += IsLive(f.S) ? 1 : 0;
                livingNeigh += IsLive(f.W) ? 1 : 0;
                livingNeigh += IsLive(f.E) ? 1 : 0;
                livingNeigh += IsLive(f.NE) ? 1 : 0;
                livingNeigh += IsLive(f.NW) ? 1 : 0;
                livingNeigh += IsLive(f.SE) ? 1 : 0;
                livingNeigh += IsLive(f.SW) ? 1 : 0;
                if (f.Live)
                {
                    if (livingNeigh < 2)
                    {
                        f.NextLive = false;
                    }
                    else if (livingNeigh <= 3)
                    {
                        f.NextLive = true;
                    }
                    else
                    {
                        f.NextLive = false;
                    }
                }
                else if (livingNeigh == 3)
                {
                    f.NextLive = true;
                }
            }
        }
        foreach (var pair in _map)
        {
            foreach (Field field in pair.Value)
            {
                field.Live = field.NextLive;
            }
        }
    }
 
    private bool IsLive(Field field)
    {
        if (field == null)
        {
            return false;
        }
        return field.Live;
    }
 
    private void Print()
    {
        Debug.WriteLine("");
        Debug.WriteLine("");
        var list = _map.OrderBy(x => x.Key).ToList();
        if (list.Any(x => x.Value.Any(f => f.Live)) == false)
        {
            Debug.WriteLine("All dead!");
            return;
        }
        // we have sorted sets
        int minX = _map.Select(x => x.Value.FirstOrDefault(f => f.Live)).Where(x => x != null).OrderBy(x => x.X).First().X;
        int maxX = _map.Select(x => x.Value.LastOrDefault(f => f.Live)).Where(x => x != null).OrderBy(x => x.X).Last().X;
        int minY = list.FirstOrDefault(x => x.Value.Any(f => f.Live)).Key;
        int maxY = list.LastOrDefault(x => x.Value.Any(f => f.Live)).Key;
        foreach (var pair in _map.OrderBy(x => x.Key))
        {
            if (pair.Key < minY || pair.Key > maxY)
            {
                continue;
            }
            StringBuilder sb = new StringBuilder();
            for (int i = minX; i <= maxX; ++i)
            {
                Field field = pair.Value.FirstOrDefault(x => x.X == i);
                if (field != null)
                {
                    char ch = field.Live ? '*' : '.';
                    sb.Append($"({ch}) ");
                }
                else
                {
                    // is death, cuz doesnt exist
                    sb.Append("(.) ");
                }
            }
            Debug.WriteLine(sb.ToString());
        }
    }
 
    private Field AddNeighbour(int x, int y)
    {
        if (_map.ContainsKey(y) == false)
        {
            _map[y] = new List<Field>();
        }
        Field existing = _map[y].FirstOrDefault(f => f.X == x);
        if (existing != null)
        {
            return existing;
        }
        existing = new Field() { X = x, Y = y, Live = false, NextLive = false };
        _map[y].Add(existing);
        return existing;
    }
}

I “pokryty” jest takim UnitTestem:

[TestMethod]
public void DailyChallenge38()
{
    DailyChallenge38 daily = new DailyChallenge38(new Tuple<int, int>[] { new Tuple<int, int>(1, 1), new Tuple<int, int>(2, 2), new Tuple<int, int>(3, 3), new Tuple<int, int>(3, 4), new Tuple<int, int>(4, 4), new Tuple<int, int>(4, 3), new Tuple<int, int>(2, 3) });
    daily.Proceed(5);
}

Cel algorytmu został opisany w zadaniu, więc nie będę się zbytnio powtarzał, generalnie chodzi o przeprowadzenie gry pt. Conway’s Game of Life.

Słabe punkty

Kod ten posiada kilka słabych punktów, są to między innymi:

  • słabe obłożenie testami jednostkowymi,
  • wszystko jest upakowane w jednej klasie: brak segregacji interfejsów, brak pojedynczej odpowiedzialności,
  • brak w nim walidacji danych wejściowych,
  • użyte są nieczytelne litania LINQ,

Pewnie znalazłoby się jeszcze więcej, ale na tych się skupię.

Rozwiązanie

Poprawmy ten kod krok po kroku. Cały kod został zmieszczony na GitHubie.

Testy jednostkowe

Na samym początku, przystępując do refactoringu, dobrze jest mieć kod pokryty testami jednostkowymi, aby widzieć czy czegoś nie popsuliśmy. Obecny kod nie jest pokryty żadnym testem jednostkowym.

Aby dobrze dokonać refaktoryzacji naprawimy ten błąd, jednak w tym celu trzeba lekko zmodyfikować kod, gdyż jego obecna forma uniemożliwia jakiekolwiek testy.

Dokonamy bardzo prostych zmian, które zapewniają, że prawie na pewno nic nie zepsujemy tj. identyfikator dostępu dla _map zmienimy na public:

public Dictionary<int, List<Field>> Map = new Dictionary<int, List<Field>>();

Mając wyniki dla 5 kroków (z wpisu DailyChallenge#38) możemy napisać taki kod testów:

[TestClass]
public class TestDailyChallenge38
{
    [TestMethod]
    public void DailyChallenge38_Step_0()
    {
        DailyChallenge38 daily = GetModel();
        Assert.AreEqual(7, GetLiving(daily));
    }
 
    [TestMethod]
    public void DailyChallenge38_Step_1()
    {
        DailyChallenge38 daily = GetModel();
        daily.Proceed(1);
        Assert.AreEqual(6, GetLiving(daily));
    }
 
    [TestMethod]
    public void DailyChallenge38_Step_2()
    {
        DailyChallenge38 daily = GetModel();
        daily.Proceed(2);
        Assert.AreEqual(4, GetLiving(daily));
    }
 
    [TestMethod]
    public void DailyChallenge38_Step_3()
    {
        DailyChallenge38 daily = GetModel();
        daily.Proceed(3);
        Assert.AreEqual(7, GetLiving(daily));
    }
 
    [TestMethod]
    public void DailyChallenge38_Step_4()
    {
        DailyChallenge38 daily = new DailyChallenge38(new Tuple<int, int>[] { new Tuple<int, int>(1, 1), new Tuple<int, int>(2, 2), new Tuple<int, int>(3, 3), new Tuple<int, int>(3, 4), new Tuple<int, int>(4, 4), new Tuple<int, int>(4, 3), new Tuple<int, int>(2, 3) });
        daily.Proceed(4);
        Assert.AreEqual(6, GetLiving(daily));
    }
 
    [TestMethod]
    public void DailyChallenge38_Step_5()
    {
        DailyChallenge38 daily = GetModel();
        daily.Proceed(5);
        Assert.AreEqual(6, GetLiving(daily));
    }
 
    private DailyChallenge38 GetModel()
    {
        return new DailyChallenge38(new Tuple<int, int>[] { new Tuple<int, int>(1, 1), new Tuple<int, int>(2, 2), new Tuple<int, int>(3, 3), new Tuple<int, int>(3, 4), new Tuple<int, int>(4, 4), new Tuple<int, int>(4, 3), new Tuple<int, int>(2, 3) }); ;
    }
 
    private int GetLiving(DailyChallenge38 daily)
    {
        int result = 0;
        foreach (var d in daily.Map)
        {
            result += d.Value.Count(x => x.Live);
        }
        return result;
    }
}

Kod najlepszy nie jest, ale przynajmniej będziemy mogli spokojniej refaktoryzować. Testy też nie są idealne, bo powinny sprawdzać koordynaty żywych pól, ale na chwilę obecną wystarczą.

Porządki w nazwach klas i plikach

Na początku nazwiemy pliki i klasy, tak aby było wiadomo do czego służą:

  • klasa Field została przeniesiona do osobnego pliku,
  • klasa DailyChallenge38 zmieniła nazwę na GameProcessor,
  • testy podążyły za zmianą nazwy i również zmieniły swoje:
    • TestDailyChallenge38 -> TestGameProcessor,
    • DailyChallenge38_Step_* -> GameProcessor_Step_*

Mapa jako osobna klasa

Aktualna mapa jest tak naprawdę słownikiem trzymanym w procesorze gry, lepiej będzie się reprezentować jako osobna klasa. Dodatkowo utworzyłem tymczasową klasę symbolizującą wiersz, tymczasową do czasu, aż nie będzie już potrzebna. Wszystkie metody związane z mapą, powinny znajdować się w nowej klasie.

Zmianom uległo kilka klasa. Klasa Field zyskała metodę GetLivingNeighbours, która zwraca ilość żywych komórek sąsiadów, oraz nadpisała metodę GetHashCode, aby była prawidłowo obsługiwana przez Hashset:

public class Field : IEquatable<Field>
{
    public int X;
    public int Y;
    public bool Live;
    public bool NextLive;
    public Field N;
    public Field W;
    public Field NW;
    public Field NE;
    public Field E;
    public Field S;
    public Field SE;
    public Field SW;
 
    public bool Equals(Field other)
    {
        return other.GetHashCode() == GetHashCode();
    }
 
    public int GetLivingNeighbours()
    {
        int livingNeigh = 0;
        livingNeigh += IsLive(N) ? 1 : 0;
        livingNeigh += IsLive(S) ? 1 : 0;
        livingNeigh += IsLive(W) ? 1 : 0;
        livingNeigh += IsLive(E) ? 1 : 0;
        livingNeigh += IsLive(NE) ? 1 : 0;
        livingNeigh += IsLive(NW) ? 1 : 0;
        livingNeigh += IsLive(SE) ? 1 : 0;
        livingNeigh += IsLive(SW) ? 1 : 0;
        return livingNeigh;
    }
 
    public override int GetHashCode()
    {
        int hashcode = 23;
        hashcode = (hashcode * 37) + X;
        hashcode = (hashcode * 37) + Y;
        return hashcode;
    }
 
    private bool IsLive(Field field)
    {
        if (field == null)
        {
            return false;
        }
        return field.Live;
    }
}

Klasa Map została utworzona, a wraz z nią klasa pomocnicza Map_Row. Do tych dwóch klas został przeniesiony kod z GameProcessora, który tam nie pasował. W rzeczywistości do tej klasy została przeniesione za dużo, ale na chwilę obecną niech tak będzie. Kod klasy Map wygląda następująco:

public class Map
{
    private Dictionary<int, Map_Row> _map = new Dictionary<int, Map_Row>();
 
    public IEnumerable<Field> Fields
    {
        get
        {
            List<Field> fields = new List<Field>();
            foreach (var row in _map)
            {
                fields.AddRange(row.Value.Fields);
            }
            return fields;
        }
    }
 
    public bool IsAnyLive
    {
        get
        {
            return _map.Any(x => x.Value.IsAnyLive);
        }
    }
 
    public Map(IEnumerable<Tuple<int, int>> lives)
    {
        foreach (var live in lives)
        {
            if (_map.ContainsKey(live.Item2) == false)
            {
                _map[live.Item2] = new Map_Row();
            }
            Field field = new Field { X = live.Item1, Y = live.Item2, Live = true, NextLive = true };
            _map[live.Item2].AddField(field);
        }
    }
 
    public void AddNeighbours()
    {
        foreach (var pair in _map.ToList())
        {
            foreach (Field field in pair.Value.Fields.ToList())
            {
                if (field.N == null)
                {
                    field.N = AddNeighbour(field.X, field.Y - 1);
                    field.N.S = field;
                }
                if (field.S == null)
                {
                    field.S = AddNeighbour(field.X, field.Y + 1);
                    field.S.N = field;
                }
                if (field.E == null)
                {
                    field.E = AddNeighbour(field.X + 1, field.Y);
                    field.E.W = field;
                }
                if (field.W == null)
                {
                    field.W = AddNeighbour(field.X - 1, field.Y);
                    field.W.E = field;
                }
                if (field.NE == null)
                {
                    field.NE = AddNeighbour(field.X + 1, field.Y - 1);
                    field.NE.SW = field;
                }
                if (field.SE == null)
                {
                    field.SE = AddNeighbour(field.X + 1, field.Y + 1);
                    field.SE.NW = field;
                }
                if (field.NW == null)
                {
                    field.NW = AddNeighbour(field.X - 1, field.Y - 1);
                    field.NW.SE = field;
                }
                if (field.SW == null)
                {
                    field.SW = AddNeighbour(field.X - 1, field.Y + 1);
                    field.SW.NE = field;
                }
            }
        }
    }
 
    public void Print()
    {
        Debug.WriteLine("");
        Debug.WriteLine("");
        if (IsAnyLive == false)
        {
            Debug.WriteLine("All dead!");
            return;
        }
        int minX = _map.Min(x => x.Value.MinLiveX);
        int maxX = _map.Max(x => x.Value.MaxLiveX);
        int minY = _map.Where(x => x.Value.IsAnyLive).Min(x => x.Key);
        int maxY = _map.Where(x => x.Value.IsAnyLive).Max(x => x.Key);
        foreach (var pair in _map.OrderBy(x => x.Key))
        {
            if (pair.Key < minY || pair.Key > maxY)
            {
                continue;
            }
            StringBuilder sb = new StringBuilder();
            for (int i = minX; i <= maxX; ++i)
            {
                Field field = pair.Value.Fields.FirstOrDefault(x => x.X == i);
                if (field != null)
                {
                    char ch = field.Live ? '*' : '.';
                    sb.Append($"({ch}) ");
                }
                else
                {
                    // is death, cuz doesnt exist
                    sb.Append("(.) ");
                }
            }
            Debug.WriteLine(sb.ToString());
        }
    }
 
    public Field AddNeighbour(int x, int y)
    {
        if (_map.ContainsKey(y) == false)
        {
            _map[y] = new Map_Row();
        }
        Field existing = _map[y].Fields.FirstOrDefault(f => f.X == x);
        if (existing != null)
        {
            return existing;
        }
        existing = new Field() { X = x, Y = y, Live = false, NextLive = false };
        _map[y].AddField(existing);
        return existing;
    }
 
    private class Map_Row
    {
        private HashSet<Field> _fields = new HashSet<Field>();
 
        public bool IsAnyLive
        {
            get
            {
                return _fields.Any(x => x.Live);
            }
        }
 
        public int MinLiveX
        {
            get
            {
                if (_fields.Any(x => x.Live) == false)
                {
                    return 0;
                }
                return _fields.Where(x => x.Live).Min(x => x.X);
            }
        }
 
        public int MaxLiveX
        {
            get
            {
                if (_fields.Any(x => x.Live) == false)
                {
                    return 0;
                }
                return _fields.Where(x => x.Live).Max(x => x.X);
            }
        }
 
 
        public IEnumerable<Field> Fields
        {
            get
            {
                return _fields;
            }
        }
 
        public void AddField(Field field)
        {
            if (_fields.Add(field) == false)
            {
                throw new ArgumentException($"Field '({field.X},{field.Y})' already exist");
            }
        }
    }
}

Kod klasy GameProcessor uległ uproszczeniu:

public class GameProcessor
{
    private Map _map;
 
    public GameProcessor(Map map)
    {
        _map = map ?? throw new ArgumentNullException();
    }
 
    public void Proceed(int n)
    {
        if (n < 0)
        {
            throw new ArgumentException();
        }
        _map.Print();
        for (int i = 0; i < n; ++i)
        {
            _map.AddNeighbours();
            MakeStep();
            _map.Print();
        }
    }
 
    private void MakeStep()
    {
        foreach (Field f in _map.Fields)
        {
            int livingNeigh = f.GetLivingNeighbours();
            if (f.Live)
            {
                if (livingNeigh < 2)
                {
                    f.NextLive = false;
                }
                else if (livingNeigh <= 3)
                {
                    f.NextLive = true;
                }
                else
                {
                    f.NextLive = false;
                }
            }
            else if (livingNeigh == 3)
            {
                f.NextLive = true;
            }
        }
        foreach (Field field in _map.Fields)
        {
            field.Live = field.NextLive;
        }
    }
}

Testy musiały ulec zmianom, ale nadal przechodzą:

[TestClass]
public class TestGameProcessor
{
 ...
    [TestMethod]
    public void GameProcessor_Step_2()
    {
        Map map = GetMap();
        GameProcessor processor = new GameProcessor(map);
        processor.Proceed(2);
        Assert.AreEqual(4, map.Fields.Count(x => x.Live));
    }
 ...
    private Map GetMap()
    {
        return new Map(new Tuple<int, int>[] { new Tuple<int, int>(1, 1), new Tuple<int, int>(2, 2), new Tuple<int, int>(3, 3), new Tuple<int, int>(3, 4), new Tuple<int, int>(4, 4), new Tuple<int, int>(4, 3), new Tuple<int, int>(2, 3) }); ;
    }
}

Kod został podzielony, bez większych zmian porządkowych.  Na razie to zostawmy, gdyż sporo się uprości po następnej zmianie.

Drukowanie mapy

Kolejnym problemem jest to, że obecnie jedyny sposób drukowania mapy polega na wypisaniu jej na Debug. Jeżeli chcielibyśmy teraz taką mapę np. wyświetlić w silniku Unity, to bez zmiany kodu mapy nie byłoby takiej możliwości. Zasada otwarte-zamknięte mówi właśnie o tym, aby kod był zamknięty na modyfikacje.

W celu naprawy został utworzony interfejs IMapPrinter oraz jego implementacja MapPrinterDebug. Kod nowych obiektów wygląda następująco:

public interface IMapPrinter
{
    void Print(Map map);
}
 
public class MapPrinterDebug : IMapPrinter
{
    public void Print(Map map)
    {
        Debug.WriteLine("");
        Debug.WriteLine("");
        if (map.IsAnyLive == false)
        {
            Debug.WriteLine("All dead!");
            return;
        }
        IEnumerable<Field> livingFields = map.Fields.Where(x => x.Live);
        int minX = livingFields.Min(x => x.X);
        int maxX = livingFields.Max(x => x.X);
        int minY = livingFields.Min(x => x.Y);
        int maxY = livingFields.Max(x => x.Y);
 
        foreach (var row in map.Fields.GroupBy(x => x.Y).OrderBy(x => x.Key))
        {
            if (row.Key < minY || row.Key > maxY)
            {
                continue;
            }
            Debug.WriteLine(PrintRow(row, minX, maxX));
        }
    }
 
    private string PrintRow(IEnumerable<Field> row, int minX, int maxX)
    {
        StringBuilder sb = new StringBuilder();
        for (int column = minX; column <= maxX; ++column)
        {
            Field field = row.FirstOrDefault(x => x.X == column);
            sb.Append($"{PrintField(field)} ");
        }
        return sb.ToString();
    }
 
    private string PrintField(Field field)
    {
        if (field != null)
        {
            char ch = field.Live ? '*' : '.';
            return $"({ch})";
        }
        else
        {
            return "(.)";
        }
    }
}

Dzięki temu metody takie jak Map.Print, Map_Row.MaxLivingX, Map_Row.MinLivingX można skasować, okazuje się, że niepotrzebnie je utworzyliśmy. Czasami tak jest, że wraz z zwiększeniem czytelności kodu, okazuje się, że część kodu można wywalić.

Po tych zmianach trzeba zmodyfikować kod UnitTestów, gdyż obecnie się nie buduje. Po zmodyfikowaniu, testy nadal przechodzą na zielono.

Reguły gry

Nasz procesor gry jest praktycznie wyczyszczony i nie ma już powodów do zmiany, poza jednym przypadkiem. Co, jeżeli reguły gry zostaną zmienione? Wtedy trzeba będzie zmieniać kod procesora. Naprawimy to tworząc interfejs IGameRules i jego domyślną implementację:

public interface IGameRules
{
    bool NeedContinue(Map map);
    bool CalculateLiveStatus(Field field);
}
 
public class GameRulesDefault : IGameRules
{
    public bool CalculateLiveStatus(Field field)
    {
        int livingNeighbours = field.GetLivingNeighbours();
        if (field.Live)
        {
            if (livingNeighbours < 2)
            {
                return false;
            }
            else if (livingNeighbours <= 3)
            {
                return true;
            }
            else
            {
                return false;
            }
        }
        return livingNeighbours == 3;
    }
 
    public bool NeedContinue(Map map)
    {
        return map.IsAnyLive;
    }
}

Reguły gry definiują dwie wartości:

  • czy w ogóle jest sens kontynuowania gry,
  • jaki jest następny status komórki.

Kod procesora gry ponownie się uprościł:

public class GameProcessor
{
    private readonly Map _map;
    private readonly IMapPrinter _printer;
    private readonly IGameRules _gameRules;
 
    public GameProcessor(Map map, IMapPrinter printer, IGameRules gameRules)
    {
        _gameRules = gameRules ?? throw new ArgumentNullException(nameof(gameRules));
        _printer = printer ?? throw new ArgumentNullException(nameof(printer));
        _map = map ?? throw new ArgumentNullException(nameof(map));
    }
 
    public void Proceed(int n)
    {
        if (n < 0)
        {
            throw new ArgumentException();
        }
        _printer.Print(_map);
        for (int i = 0; i < n; ++i)
        {
            if (_gameRules.NeedContinue(_map) == false)
            {
                return;
            }
            _map.AddNeighbours();
            MakeStep();
            _printer.Print(_map);
        }
    }
 
    private void MakeStep()
    {
        Dictionary<Field, bool> nextLives = new Dictionary<Field, bool>();
        foreach (Field f in _map.Fields)
        {
            nextLives.Add(f, _gameRules.CalculateLiveStatus(f));
        }
        foreach (var pair in nextLives)
        {
            pair.Key.Live = pair.Value;
        }
    }
}

Okazało się również, że property Field.NextLive jest zbędna, więc została usunięta.

Ostatnie czystki

Mając już kod na takim etapie, możemy sobie pozwolić na końcowe szlify w projekcie.

Pierwszym będzie dodanie domyślnego obiektu dla obiektu drukującego mapę. Obiekt drukujący mapę nie powinien być obowiązkowy, gdyż ktoś może nie chcieć tej mapy przecież wyświetlać. Dopuśćmy obiekt pusty, ale zamiast za każdym razem sprawdzać w procesorze, czy obiekt jest nullem, lepiej utworzyć pustą implementację obiektu:

class MapPrinterEmpty : IMapPrinter
{
    public void Print(Map map)
    {
    }
}

I przekazać ją jako domyślną w konstruktorze procesora:

public GameProcessor(Map map, IGameRules gameRules, IMapPrinter printer = null)
{
    _gameRules = gameRules ?? throw new ArgumentNullException(nameof(gameRules));
    _map = map ?? throw new ArgumentNullException(nameof(map));
    _printer = printer ?? new MapPrinterEmpty();
}

Dodatkowo klasa Map_Row jest teraz zbędna, mapę można reprezentować jako „podwójny słownik”, co z kolei wpływa klasę Field, która już nie musi trzymać referencji na sąsiadów w celach optymalizacyjnych. Dodatkowo zostały uproszone wszystkie operacje wykonywana na sąsiadach danego pola. Również dodawanie nowych sąsiadów, zostało ograniczone tylko dla żywych komórek, gdyż nie ma sensu tego robić dla martwych, co dodatkowo optymalizuje algorytm. Klasa Map wygląda teraz następująco:

public class Map
{
    private Dictionary<int, Dictionary<int, Field>> _map = new Dictionary<int, Dictionary<int, Field>>();
 
    public IEnumerable<Field> Fields
    {
        get
        {
            List<Field> fields = new List<Field>();
            foreach (var row in _map)
            {
                fields.AddRange(row.Value.Values);
            }
            return fields;
        }
    }
 
    public bool IsAnyLive => _map.Any(x => x.Value.Values.Any(f => f.Live));
 
    public Map(IEnumerable<Tuple<int, int>> lives)
    {
        if (lives != null)
        {
            foreach (var live in lives)
            {
                if (_map.ContainsKey(live.Item2) == false)
                {
                    _map[live.Item2] = new Dictionary<int, Field>();
                }
                Field field = new Field { X = live.Item1, Y = live.Item2, Live = true };
                _map[live.Item2][live.Item1] = field;
            }
        }
    }
 
    public int CalculateLivingNeighbours(Field field)
    {
        if (field == null)
        {
            throw new ArgumentNullException(nameof(field));
        }
        int livingNeigh = 0;
        DoForAllNeighbours(field, (x, y) =>
        {
            livingNeigh += IsLive(x, y) ? 1 : 0;
        });
        return livingNeigh;
    }
 
    public void AddNeighbours()
    {
        foreach (Field field in Fields.Where(x => x.Live).ToList())
        {
            DoForAllNeighbours(field, (x, y) => AddField(x, y));
        }
    }
 
    private void DoForAllNeighbours(Field field, Action<int, int> action)
    {
        for (int x = field.X - 1; x <= field.X + 1; x++)
            for (int y = field.Y - 1; y <= field.Y + 1; y++)
            {
                if (x == field.X && y == field.Y)
                {
                    continue;
                }
                action(x, y);
            }
    }
 
    private void AddField(int x, int y)
    {
        if (_map.ContainsKey(y) == false)
        {
            _map[y] = new Dictionary<int, Field>();
        }
        if (_map[y].ContainsKey(x) == false)
        {
            _map[y][x] = new Field() { X = x, Y = y, Live = false };
        }
    }
 
    private bool IsLive(int x, int y)
    {
        if (_map.TryGetValue(y, out Dictionary<int, Field> row))
        {
            if (row.TryGetValue(x, out Field field))
            {
                return field.Live;
            }
        }
        return false;
    }
}

Z racji, że metoda CalculateLivingNeighbours znajduje się teraz w klasie Map zmianie uległ interfejs IGameRules, klasa Field i kod GameProcessor:

public class Field
{
    public int X;
    public int Y;
    public bool Live;
}
 
public interface IGameRules
{
    bool NeedContinue(Map map);
    bool CalculateLiveStatus(Field field, int livingNeighbours);
}
 
public class GameProcessor
{
...
    private void MakeStep()
    {
        _map.AddNeighbours();
        Dictionary<Field, bool> nextLives = new Dictionary<Field, bool>();
        foreach (Field field in _map.Fields)
        {
            bool nextStatus = _gameRules.CalculateLiveStatus(field, _map.CalculateLivingNeighbours(field));
            nextLives.Add(field, nextStatus);
        }
        foreach (var pair in nextLives)
        {
            pair.Key.Live = pair.Value;
        }
        _printer.Print(_map);
    }
...
}

Podsumowanie

I tym oto sposobem dobrnęliśmy do końca. Jest jeszcze jedna rzecz, której tutaj nie zaprezentowałem, a wypadało by zrobić. Trzeba napisać testy jednostkowe dla nowo powstałych klas. Pozostawiam Ci to jako zadanie domowe, jeżeli jesteś leniwy, to na GitHubie znajdują się już zaimplementowane testy 😉.

Reasumując, przeprowadzając ten refactoring na początku podzieliliśmy kod, tak aby był bardziej czytelny. W kolejnych etapach sugerowaliśmy się zasadami SOLID oraz wykorzystaliśmy takie wzorce:

Taki refactoring był możliwy dzięki początkowemu obłożeniu kodu testami jednostkowymi, które będąc nawet “niedorobione” nam pomogły.

Zaznaczam, że przedstawione tutaj rozwiązanie nie jest jedynym, ktoś inny może zaproponować inne. Nie musi być ono wcale lepsze albo gorsze, po prostu inne. Każdy pisze tak jakie ma nawyki. W tym przykładzie równie dobrze można by wykorzystać wzorzec obserwator jako sposób na drukowanie mapy. Kwestia upodobań.

Jak pewnie zauważyłeś, podczas prac tworzyliśmy klasy, które później usuwaliśmy. Jest to normalny proces, a przynajmniej normalny, jak nie rozplanujesz wszystkiego od początku, przy tak małej skali kodu nie chciałem wszystkiego planować, wolałem uporządkować kod po kroku. Przy większym projekcie dobrze jest wszystko chociaż z grubsza zaplanować, aby uniknąć niepotrzebnej pracy.

Dzięki za przeczytanie artykułu, mam nadzieję, że się podobał. Do następnego razu!

Patryk Bogdański

Dodaj komentarz

Twój adres email nie zostanie opublikowany. Pola, których wypełnienie jest wymagane, są oznaczone symbolem *

This site uses Akismet to reduce spam. Learn how your comment data is processed.