Direkt zum Hauptbereich

Refactoring II: Politik der kleinen Schritte


Bevor ich von Refactoring hörte, hieß die Antwort auf schlechte Wartbarkeit (Weiterentwicklung, Fehlerbereinigung) und Performanceprobleme von Softwarekomponenten Redesign: Es wurden Klassendiagramme und ggf. Aktivitätendiagramme des Ist-Zustands erstellt und auf diesem Abstraktionsniveau dann überlegt, wie das Zieldesign aussehen soll. Dann wurde versucht, diesen Entwurf mit einem mehr oder weniger großen Ruck in Sourcecode zu gießen.

Beim Refactoring bleiben wir auf dem Niveau des Sourcecodes; und wir machen kleine Schritte. Aber lassen wir die Theorie und schauen wir uns einfach mal ein Refactoring im Beispiel an.

Legen wir los! Vielleicht stoße ich bei der Lektüre einer mir fremden Quelldatei auf die folgende Methode:
public void Buy(String x)
{
   if ((Int32.Parse(x[0]) * 1
     + Int32.Parse(x[1]) * 2
     + Int32.Parse(x[2]) * 3
     + Int32.Parse(x[3]) * 4
     + Int32.Parse(x[4]) * 5
     + Int32.Parse(x[5]) * 6
     + Int32.Parse(x[6]) * 7
     + Int32.Parse(x[7]) * 8
     + Int32.Parse(x[8]) * 9
     + Int32.Parse(x[9]=='X'?"10":x[9].ToString()) * 10) % 11
     == 0)
   {
      string s;
      double p1 = AskForPrice(x, "Super Shop");
      double p2 = AskForPrice(x, "The Book Shop");
      double p3 = AskForPrice(x, "Your Shop");
      double p4 = AskForPrice(x, "No Name Shop");
      if (p1 <= p2 && p1 <= p3 && p1 <= p4)
      {
         s = "Super Shop";
      }
      else
      {
         if (p2 <= p1 && p2 <= p3 && p2 <= p4)
         {
            s = "The Book Shop";
         }
         else
         {
            if (p3 <= p1 && p3 <= p2 && p3 <= p4)
            {
               s = "Your Shop";
            }
            else
            {
               s = "No Name Shop";
            }
         }
      }
     
      SendBookOrder(x, s);
   }
   else
   {
      ShowErrorMessage("The ISBN " + x + " isn't valid.");
   }
}
Welche Gedanken kommen mir beim Lesen? Der Name ist "Buy" also kaufe ich mit der Methode etwas? Was? x? Was ist x? Mal schauen. Als erstes wird irgendetwas geprüft. Der String wird zerlegt und es wird was gerechnet? Naja, schau ich mir später genauer an. Was passiert denn, wenn die Bedingung erfüllt ist?

Ah, da wird die Funktion "AskForPrice" mehrmals aufgerufen. Vermutlich liefert mir diese Funktion einen Preis zurück. Ich prüfe mal die Dokumentation der Funktion "AskForPrice". (Wenn ich Glück habe, ist die Schnittstelle der Funktion "AskForPrice" gut dokumentiert. Wenn nicht, muss ich erst diese Funktion weiter untersuchen, damit ich weiß, was sie mir wirklich zurück liefert und was sie an Parametern verlangt.) Laut deren Doku liefert sie den Preis zurück und will als Parameter zum einen eine ISBN - ah, x ist also eine ISBN - und den Namen eines Geschäfts, dass nach den Preis gefragt wird.

Na dann benennen wir doch mal x in isbn und p1 bis p4 in price1 bis price4 um:
public void Buy(String isbn)
{
   if ((Int32.Parse(isbn[0].ToString()) * 1
     + Int32.Parse(isbn[1].ToString()) * 2
     + Int32.Parse(isbn[2].ToString()) * 3
     + Int32.Parse(isbn[3].ToString()) * 4
     + Int32.Parse(isbn[4].ToString()) * 5
     + Int32.Parse(isbn[5].ToString()) * 6
     + Int32.Parse(isbn[6].ToString()) * 7
     + Int32.Parse(isbn[7].ToString()) * 8
     + Int32.Parse(isbn[8].ToString()) * 9
     + Int32.Parse(isbn[9]=='X'?"10":isbn[9].ToString()) * 10) % 11
     == 0)
   {
      string s;
      double price1 = AskForPrice(isbn, "Super Shop");
      double price2 = AskForPrice(isbn, "The Book Shop");
      double price3 = AskForPrice(isbn, "Your Shop");
      double price4 = AskForPrice(isbn, "No Name Shop");
      if (price1 <= price2 && price1 <= price3 && price1 <= price4)
      {
         s = "Super Shop";
      }
      else
      {
         if (price2 <= price1 && price2 <= price3 && price2 <= price4)
         {
            s = "The Book Shop";
         }
         else
         {
            if (price3 <= price1 && price3 <= price2 && price3 <= price4)
            {
               s = "Your Shop";
            }
            else
            {
               s = "No Name Shop";
            }
         }
      }
     
      SendBookOrder(isbn, s);
   }
   else
   {
      ShowErrorMessage("The ISBN " + isbn + " isn't valid.");
   }
}

Als nächstes werden die Preise miteinander verglichen. Sieht so aus, als ob der niedrigste Preis ermittelt werden soll. Und dann wird anscheinend s jeweils auf den Namen des Geschäfts gesetzt, welches den niedrigsten Preis geliefert hat. Was wird mit s noch gemacht? Es geht in die Methode "SendBookOrder". Dokumentation der Methode prüfen! Ja richtig. Die will den Namen des Shops. Na dann benennen wir mal s in shopName um:
public void Buy(String isbn)
{
   if ((Int32.Parse(isbn[0].ToString()) * 1
     + Int32.Parse(isbn[1].ToString()) * 2
     + Int32.Parse(isbn[2].ToString()) * 3
     + Int32.Parse(isbn[3].ToString()) * 4
     + Int32.Parse(isbn[4].ToString()) * 5
     + Int32.Parse(isbn[5].ToString()) * 6
     + Int32.Parse(isbn[6].ToString()) * 7
     + Int32.Parse(isbn[7].ToString()) * 8
     + Int32.Parse(isbn[8].ToString()) * 9
     + Int32.Parse(isbn[9]=='X'?"10":isbn[9].ToString()) * 10) % 11
     == 0)
   {
      string shopName;
      double price1 = AskForPrice(isbn, "Super Shop");
      double price2 = AskForPrice(isbn, "The Book Shop");
      double price3 = AskForPrice(isbn, "Your Shop");
      double price4 = AskForPrice(isbn, "No Name Shop");
      if (price1 <= price2 && price1 <= price3 && price1 <= price4)
      {
         shopName = "Super Shop";
      }
      else
      {
         if (price2 <= price1 && price2 <= price3 && price2 <= price4)
         {
            shopName = "The Book Shop";
         }
         else
         {
            if (price3 <= price1 && price3 <= price2 && price3 <= price4)
            {
               shopName = "Your Shop";
            }
            else
            {
               shopName = "No Name Shop";
            }
         }
      }   
  
      SendBookOrder(isbn, shopName);
   }
   else
   {
      ShowErrorMessage("The ISBN " + isbn + " isn't valid.");
   }
}

Ok, jetzt weiß ich, glaube ich, was diese Methode so macht. Wenn die obige Bedingung fehlschlägt, wird ja auch eine Fehlermeldung ausgegeben, die... ok, die Bedingung oben prüft also, ob es sich um eine gültige ISBN handelt. Zeit, die Methode mal etwas zu verkleinern. Ich packe die Prüfbedingung in eine eigene Methode, mit dem schönen Namen "IsValidIsbn". Dann müsste gleich klar sein, was die macht:
public void Buy(String isbn)
{
   if (IsValidIsbn(isbn))
   {
      string shopName;
      double price1 = AskForPrice(isbn, "Super Shop");
      double price2 = AskForPrice(isbn, "The Book Shop");
      double price3 = AskForPrice(isbn, "Your Shop");
      double price4 = AskForPrice(isbn, "No Name Shop");
      if (price1 <= price2 && price1 <= price3 && price1 <= price4)
      {
         shopName = "Super Shop";
      }
      else
      {
         if (price2 <= price1 && price2 <= price3 && price2 <= price4)
         {
            shopName = "The Book Shop";
         }
         else
         {
            if (price3 <= price1 && price3 <= price2 && price3 <= price4)
            {
               shopName = "Your Shop";
            }
            else
            {
               shopName = "No Name Shop";
            }
         }
      }      

      SendBookOrder(isbn, shopName);
   }
   else
   {
      ShowErrorMessage("The ISBN " + isbn + " isn't valid.");
   }
}

private bool IsValidIsbn(String isbn)
{
   return (Int32.Parse(isbn[0].ToString()) * 1
        + Int32.Parse(isbn[1].ToString()) * 2
        + Int32.Parse(isbn[2].ToString()) * 3
        + Int32.Parse(isbn[3].ToString()) * 4
        + Int32.Parse(isbn[4].ToString()) * 5
        + Int32.Parse(isbn[5].ToString()) * 6
        + Int32.Parse(isbn[6].ToString()) * 7
        + Int32.Parse(isbn[7].ToString()) * 8
        + Int32.Parse(isbn[8].ToString()) * 9
        + Int32.Parse(isbn[9]=='X'?"10":isbn[9].ToString()) * 10) % 11
        == 0);
}
Und auch der Teil, welcher den Namen des Shops zurück gibt, der den niedrigsten Preis hat, schreit danach in eine eigene Methode ausgegliedert zu werden. Tu ich ihm den Gefallen:
public void Buy(String isbn)
{
   if (IsValidIsbn(isbn))
   {
      string shopName = GetShopWithLowestPrice(isbn);
     
      SendBookOrder(isbn, shopName);
   }
   else
   {
      ShowErrorMessage("The ISBN " + isbn + " isn't valid.");
   }
}

private string GetShopWithLowestPrice(string isbn)
{
   string shopName;
   double price1 = AskForPrice(isbn, "Super Shop");
   double price2 = AskForPrice(isbn, "The Book Shop");
   double price3 = AskForPrice(isbn, "Your Shop");
   double price4 = AskForPrice(isbn, "No Name Shop");
   if (price1 <= price2 && price1 <= price3 && price1 <= price4)
   {
      shopName = "Super Shop";
   }
   else
   {
      if (price2 <= price1 && price2 <= price3 && price2 <= price4)
      {
         shopName = "The Book Shop";
      }
      else
      {
         if (price3 <= price1 && price3 <= price2 && price3 <= price4)
         {
            shopName = "Your Shop";
         }
         else
         {
            shopName = "No Name Shop";
         }
      }
   } 
   return shopName;
}

private bool IsValidIsbn(String isbn)
{
   return (Int32.Parse(isbn[0].ToString()) * 1
        + Int32.Parse(isbn[1].ToString()) * 2
        + Int32.Parse(isbn[2].ToString()) * 3
        + Int32.Parse(isbn[3].ToString()) * 4
        + Int32.Parse(isbn[4].ToString()) * 5
        + Int32.Parse(isbn[5].ToString()) * 6
        + Int32.Parse(isbn[6].ToString()) * 7
        + Int32.Parse(isbn[7].ToString()) * 8
        + Int32.Parse(isbn[8].ToString()) * 9
        + Int32.Parse(isbn[9]=='X'?"10":isbn[9].ToString()) * 10) % 11
        == 0);
}
Na jetzt sieht unsere "Buy" Methode doch schön übersichtlich aus. "Buy" scheint mir aber nicht der richtige Name zu sein. Ich glaube ich würde ihn in "OrderCheapestBook" ändern, und die Variable shopName brauche ich auch nicht mehr wirklich:
public void OrderCheapestBook(String isbn)
{
   if (IsValidIsbn(isbn))
   {
      SendBookOrder(isbn, GetShopWithLowestPrice(isbn));
   }
   else
   {
      ShowErrorMessage("The ISBN " + isbn + " isn't valid.");
   }
}
Wenden wir uns nun noch den ausgegliederten Methoden zu. Beginnen wir mit "GetShopWithLowestPrice". Alle Pfade setzen den shopName, welcher dann am Ende zurückgegeben wird. Die Rückgabe kann eigentlich auch jeweils sofort erfolgen. Dann spare ich mir die else-Zweige und die Variable shopName:
private string GetShopWithLowestPrice(string isbn)
{
   double price1 = AskForPrice(isbn, "Super Shop");
   double price2 = AskForPrice(isbn, "The Book Shop");
   double price3 = AskForPrice(isbn, "Your Shop");
   double price4 = AskForPrice(isbn, "No Name Shop");
   if (price1 <= price2 && price1 <= price3 && price1 <= price4)
   {
      return "Super Shop";
   }
   if (price2 <= price1 && price2 <= price3 && price2 <= price4)
   {
      return "The Book Shop";
   }
   if (price3 <= price1 && price3 <= price2 && price3 <= price4)
   {
      return "Your Shop";
   }
   return "No Name Shop";
}
Dennoch hat diese Funktion noch einen großen Makel. Sie sieht sehr stark danach aus, dass sie oft geändert werden könnte. Wenn sich der Name eines Geschäfts ändert, muss ich als Entwickler daran denken, dass dieser Name dann an zwei Stellen geändert werden muss. Riskant! Wenn ein neuer Shop hinzukommt, oder ein existierender entfernt wird, darf ich auch immer alle If-Bedingungen anpassen. Nein, das gefällt mir gar nicht.

Das ganze ruft nach einer Liste. Hier trickse ich jetzt rum und greife einfach auf die inzwischen mächtigen Listenverarbeitungen der Standardbibliotheken zurück. In meinem Fall, die von .NET:
private string GetShopWithLowestPrice(string isbn)
{
   var shopNames = new[] {
         "Super Shop",
         "The Book Shop",
         "Your Shop",
         "No Name Shop"};
  
   return shopNames.OrderBy(name => AskForPrice(isbn, name)).First();
}
Jetzt sind Änderungen für diese Funktion kein Problem mehr. Nur die Liste muss angepasst werden. Jetzt eröffnet sich hier auch die Möglichkeit, dass die Namen aus einer Konfigurationsdatei geladen werden können. Das würde die Software viel flexibler machen. Aber wir sind jetzt nicht dabei, neue Funktionen umzusetzen. Refactoring hat uns jetzt aber die Möglichkeit gegeben (und aufgezeigt) dies leichter zu tun.

Gehen wir zur "IsValidIsbn" Funktion. Hier gefällt mir zuerst einmal nicht, dass die Behandlung der zehnten Stelle so aus der Reihe tanzt. Sonderfälle sind immer ärgerlich. Die ersten neun Stellen scheinen immer Ziffern zu sein, die konvertiert werden können. Nur die zehnte Stelle kann anscheinend auch ein "X" enthalten, welches dann als "10" behandelt werden muss. Mhh...mhh...
Nicht die zehnte Stelle ist der Sonderfall, sondern die ersten neun Stellen sind der Sonderfall für die allgemeinere Behandlung der zehnten Stelle. Also kann ich die ersten neun Stellen mit dem gleichen Algorithmus behandeln wie die zehnte Stelle.

Vorher extrahiere ich diesen Check mit dem anschließenden Parsen in eine extra Funktion:
private int ParseIsbnDigit(char digit)
{
   return Int32.Parse(digit == 'X' ? "10" : digit.ToString());
}
Diese Funktion wende ich jetzt auf alle Stellen an:
private bool IsValidIsbn(String isbn)
{
   return (
          ParseIsbnDigit(isbn[0]) * 1
        + ParseIsbnDigit(isbn[1]) * 2
        + ParseIsbnDigit(isbn[2]) * 3
        + ParseIsbnDigit(isbn[3]) * 4
        + ParseIsbnDigit(isbn[4]) * 5
        + ParseIsbnDigit(isbn[5]) * 6
        + ParseIsbnDigit(isbn[6]) * 7
        + ParseIsbnDigit(isbn[7]) * 8
        + ParseIsbnDigit(isbn[8]) * 9
        + ParseIsbnDigit(isbn[9]) * 10)
        % 11 == 0);
}
Ich höre schon wieder "Liste" rufen. Bitte:
private bool IsValidIsbn(String isbn)
{
   return isbn
      .Select((digit, index) => ParseIsbnDigit(digit) * (index + 1))
      .Sum() % 11 == 0;
}
Schauen wir uns das Endergebnis in der Gesamtsicht an:
public void OrderCheapestBook(String isbn)
{
   if (IsValidIsbn(isbn))
   {
      SendBookOrder(isbn, GetShopWithLowestPrice(isbn));
   }
   else
   {
      ShowErrorMessage("The ISBN " + isbn + " isn't valid.");
   }
}

private string GetShopWithLowestPrice(string isbn)
{
   var shopNames = new[] {
         "Super Shop",
         "The Book Shop",
         "Your Shop",
         "No Name Shop"};
  
   return shopNames.OrderBy(name => AskForPrice(isbn, name)).First();
}

private bool IsValidIsbn(String isbn)
{
   return isbn
      .Select((digit, index) => ParseIsbnDigit(digit) * (index + 1))
      .Sum() % 11 == 0;
}

private int ParseIsbnDigit(char digit)
{
   return Int32.Parse(digit == 'X' ? "10" : digit.ToString());
}
Ob diese Form nun verständlicher und leichter wartbar ist als die Ausgangsform, muss der Leser selbst beurteilen.

Noch einige Worte zum Schluss: Hätten Kommentare es nicht auch getan?

Ein unverständlicher Quelltext mit Kommentaren ist natürlich einem unverständlichen Quelltext ohne Kommentare vorzuziehen. Kommentare sind nicht das Übel, aber sie können ein Indikator dafür sein, dass der Quelltext so schlecht ist, dass er ohne Kommentare nicht zu verstehen ist. In diesem Fall sollte man so weit refaktorisieren, bis man die Kommentare nicht mehr benötigt.

Allgemein sollten Kommentare nicht erklären, was gemacht wird - das sollte der Quelltext tun. Kommentare sind jedoch wichtig, um zu dokumentieren warum etwas gemacht wurde. So würde es sich zum Beispiel anbieten, in die Methode "IsValidIsbn" ein Kommentar einzufügen, dass eine Referenz auf die ISBN-Spezifikation enthält, oder sonst wie erklärt, warum man so und nicht anders, die ISBN auf Gültigkeit prüft.

Kommentare

Beliebte Posts aus diesem Blog

Silvesternachlese

Ich hoffe Ihr seit alle gut ins Jahr 2019 gekommen.

Im Vorfeld von Silvester gab es ja einige Diskussionen über die Umweltverträglichkeit, die Gesundheitsgefährdung und die finanzielle Sinnhaftigkeit von pyrotechnischen Utensilien.

Auch wir hatten über einige Alternativen nachgedacht. Letztendlich müssen zwei Eigenschaften nachgebildet werden: Zum einen muss es Knallen (oder zischen oder knattern; also laute Geräusche produzieren) und zum anderen muss es blinken. Nichts, was man heutzutage nicht mit Lautsprechern und LEDs hinbekommen könnte.

Gut, für'n ordentlichen Bums braucht man schon einigermaßen gute Lautsprecher. Aber hier könnte man ja jeden Zuschauer Kopfhörer verpassen, was auch die Umweltlärmbelastung verringern würde.

Die Lichtanlage in den Himmel zu bekommen, um ein Höhenfeuerwerk nachzustellen wird schon schwieriger. Hier könnten aber langfristig VR-Brillen Abhilfe schaffen.

Bis dieser technologische Stand erreicht ist, müssen jedoch andere Mittel herangezogen werden.…

RUCK ZUCK

In einem seiner Kommentare verwies Steinchen auf die 90er Jahre Hymne von Böhmermann (das erste Mal übrigens, dass ich etwas von Böhmermann gesehen habe). Darin kommt auch die Textzeile "...haben wir RUCK ZUCK einhundert Leute befragt..." vor.

Da dämmerte mir etwas. Stimmt, da gab's doch so eine Spielshow. Ich habe gleich mal bei YouTube recherchiert und das folgende Video gefunden. Daraufhin fiel mir ein, dass wir das sogar mindestens einmal auf dem Schulhof nachgespielt hatten. Das kann aber erst nach 1991 gewesen sein.

Im Labyrinth

Anfang Februar haben wir das schöne Wetter genutzt und sind in den Park gegangen, solange er noch unentgeltlich offen ist. Dort haben wir dann die Kinder ins Labyrinth gesteckt, mit dem Auftrag uns dort drinnen zu suchen. Wir sind währenddessen ins Café gegangen.

"Habt ihr euch aber gut versteckt," meinten sie dann, als wir sie nach einer halben Stunden wieder abholten.

Der folgende Filmbeitrag dient dazu, den Zuschauer schwindelig zu machen. Auch deswegen habe ich entgegenkommende Personen unkenntlich gemacht.