Refactoring: FizzBuzz

Schauen Sie sich bitte folgende Implementierung der Kata Fizzbuzz etwas genauer an. Diese Version ist voll funktionsfähig und erfüllt alle Tests aber es gibt ein paar Dinge die uns hier auffallen müssten.

public class FizzBuzz
{
    public static string PrintFizzBuzz()
    {
        var resultFizzBuzz = string.Empty;
        resultFizzBuzz = GetNumbers(resultFizzBuzz);
        return resultFizzBuzz;
    }

    public static string PrintFizzBuzz(int number)
    {
        var result = GetFizzBuzzResult(number);

        if (string.IsNullOrEmpty(result))
            result = GetFizzResult(number);
        if (string.IsNullOrEmpty(result))
            result = GetBuzzResult(number);

        return string.IsNullOrEmpty(result) ? number.ToString() : result;
    }

    private static string GetFizzBuzzResult(int number)
    {
        string result = null;
        if (IsFizz(number) && IsBuzz(number)) result = "FizzBuzz";
        return result;
    }

    private static string GetBuzzResult(int number)
    {
        string result = null;
        if (IsBuzz(number)) result = "Buzz";
        return result;
    }

    private static string GetFizzResult(int number)
    {
        string result = null;
        if (IsFizz(number)) result = "Fizz";
        return result;
    }

    private static string GetNumbers(string resultFizzBuzz)
    {
        for (var i = 1; i <= 100; i++)
        {
            var printNumber = string.Empty;
            if (IsFizz(i)) printNumber += "Fizz";
            if (IsBuzz(i)) printNumber += "Buzz";
            if (IsNumber(printNumber))
                printNumber = (i).ToString();
            resultFizzBuzz += " " + printNumber;
        }
        return resultFizzBuzz.Trim();
    }

    private static bool IsNumber(string printNumber)
    {
        return String.IsNullOrEmpty(printNumber);
    }

    private static bool IsBuzz(int i)
    {
        return i % 5 == 0;
    }

    private static bool IsFizz(int i)
    {
        return i % 3 == 0;
    }
}
  • Als aller erstes könnte Ihnen Aufgefallen sein das alle Methode static sind. Warum das hier so sein muss wissen wir nicht mit Sicherheit aber es könnte einfach eine unbedachte Umsetzung des ReSharper Hinweises gewesen sein. static ist nicht böse aber wie wir Entwickler nun mal sind missbrauchen wir es ab und zu weil es vielleicht einfacher zu benutzen ist (keine Instanziierung). Sie sollten static mit Bedacht verwenden, denn nur zu schnell wird aus einfachen Code komplexer Code und dann steht man vielleicht vor einen der folgenden Probleme mit static: Vererbung, Interfaces, Tests/Mocks.

  • Namen: die „IsNumber“ Methode lässt vom Namen, Parametern und Rückgabewerte darauf schließen dass der übergebene string auf Nummern geprüft wird. Nur leider macht diese Funktion das überhaupt nicht, sondern prüft nur ob der string Null oder Leer ist. Es könnte daher leicht passieren, dass jemand anders diese Methode wieder verwenden möchte und durch das falsche Verhalten auf die Nase fällt. 

  • Duplizierter Code: Ja selbst in der kleinsten Klasse kann man auf duplizierten Code stoßen. Die „IsBuzz“ und „IsFizz“ haben den gleichen Code, es ist allein der Divisor unterschiedlich. Wir können also einfach den Divisor als Parameter einfügen und benötigen nur noch eine Methode für alle Divisionen (wenn man dafür unbedingt eine extra Methode will/brauch). 

    private bool ModuloZero(int dividend, int divisor)
    {
        return dividend % divisor == 0;
    }
  • Duplizierter Code: Es fällt nun auf das auch die Mehoden „GetBuzzResult“ und „GetFizzResult“ nun den gleichen Code enthalten. Hier können wir wie im vorherigen Beispiel auch wieder die unterschiedlichen fixen Werte als Parameter extrahieren. Sehr sinnvoll sind diese Methoden hier allerdings alle nicht, weil Sie eigentlich nur aus einer einzigen If-Bedingung bestehen.

    private string GetFizzBuzzResult(int number)
    {
        string result = null;
        if (ModuloZero(number, 3) && ModuloZero(number, 5)) result = "FizzBuzz";
        return result;
    }
    
    private string GetResult(int number, int dividend, string txt)
    {
        string result = null;
        if (ModuloZero(number, dividend)) result = txt;
        return result;
    }
  • Performance: In der „PrintFizzBuzz“ Methode wird als erstes „GetFizzBuzzResult“ aufgerufen was wie wir jetzt genau wissen unsere neue Modulo Methode mit 3 und 5 aufruft. Danach wird noch mal über die „GetResult“ Methode Modulo mit 3 und 5 aufgerufen. Das bedeutet das bei jedem Aufruf dieser Funktion wird Modulo 4-mal und „IsNullOrEmpty“ 3-mal aufgerufen. Diese Methoden brauchen natürlich nicht lange und kein Benutzer hätte Probleme mit der Performance aber es verschwendet unnötige Ressourcen (CPU-Zeit) und könnte uns bei späteren Änderungen viel Ärger bereiten (ersetzt Sie doch mal die Modulo Methode durch eine Datenbankabfrage). Wie können wir also die Methode effektiver machen? Als aller erstes wäre es logisch, wenn „GetFizzBuzzResult“ etwas liefert, mit Return weitere unnötige Berechnungen zu verhindern. Wenn aber die Nummer nicht „FizzBuzz“ ist müssen wir immer noch 2 weitere male Modulo Aufrufen. Daher ist „GetFizzBuzzResult“ eh überflüssig weil wir ja später schon Modulo aufrufen werden und uns nur die Ergebnisse merken müssten.

  • Duplizierte Logik: „GetNumbers“ macht genau das was wir eigentlich in der „PrintFizzBuzz“ benötigen um effektiver zu sein. Es wird Modulo nur 2-mal aufgerufen und einmal „IsNullOrEmpty“ geprüft, leider wird es hier in eine Schleife 100-mal getan. Um nun genau diese Logik von „GetNumbers“ auch in „PrintFizzBuzz“ wieder zu verwenden müssen wird den Teil extrahieren und dann in beiden Methoden aufrufen. 

  • Zum Schluss vielleicht noch ein paar kleine aber effektive Änderungen: feste Werte die wild im Code verstreut sind, werden gerne bei Änderungen übersehen bzw. verhindern Sie auch, dass wir uns schnell/einfach auf neue Anforderungen reagieren können. In unserem Beispiel wären das z.b. die Zahlen 3 & 5 und die dazugehörigen Texte „Fizz“ & „Buzz“. Wenn Sich jetzt eine der Zahlen oder Texte ändert, müssen wir viele kleine Änderungen im Code machen und es besteht ein nicht unerhebliches Risiko die eine oder andere Stelle zu vergessen. Die einfachste Lösung wäre es diese festen Werte als const zu speichern und diese zu verwenden. Wenn sich jetzt aber wirklich das Verhalten ändern sollte ( „Fizz“ soll bei Zahlen die durch 2 teilbar sind angezeigt werden) oder es kommt eine komplett neue Abfrage (bei Teilbar durch 7 soll „Bar“ angezeigt werden) hinzu, hilft uns const nicht mehr weiter. Denn diese Verbindungen zwischen den Zahlen und den Texten sind auch fest im Code hinterlegt. Mit einem einfachen Dictionary, in dem Zahlen und Texte gespeichert sind, können wir alle festen Werte an einer Stelle definieren. Dadurch müssen wir bei Änderungen nun nur noch eine zentrale Stelle ändern und müssen bei neuen Abfragen keine einzige Zeile unserer Programm Logik ändern. (Diese Definition könnte natürlich auch von außen als Parameter übergeben werden oder als „config“ gespeichert sein)

Ergebnis:

public class Refactored
{
    private readonly IDictionary<int, string> m_Mapping;

    public Refactored()
    {
        m_Mapping = new Dictionary<int, string>();
        m_Mapping.Add(3, "Fizz");
        m_Mapping.Add(5, "Buzz");
    }

    public string FizzBuzz(int number)
    {
        return DoFizzBuzz(number);
    }

    private string Range(int count)
    {
        string result = string.Empty;

        for (var i = 1; i <= count; i++)
        {
            result += " " + DoFizzBuzz(i);
        }

        return result.Trim();
    }

    private string DoFizzBuzz(int number)
    {
        var printNumber = string.Empty;

        foreach (var mapping in m_Mapping)
        {
            if (number % mapping.Key == 0) printNumber += mapping.Value;
        }
            
        if (string.IsNullOrEmpty(printNumber)) printNumber = (number).ToString();
            
        return printNumber;
    }

    public string FizzBuzzRange(int count)
    {
        return Range(count);
    }
}