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.
Labels:

Kommentare

Kommentar veröffentlichen

Beliebte Posts aus diesem Blog

I see u

  Beim alljährlichen erzwungenen Aufräumen fand sich dieses Bild. Das älteste Kind hatte es vor etwa ein oder zwei Jahren gemalt und dann an die Tür des zweitältesten Kindes geklebt. Lieb, nicht?
1 Kommentar
Mehr anzeigen

Keine Angst vorm Manchester-Liberalismus

Ich recherchiere gerade etwas über das 19. Jahrhundert und die Industrielle Revolution, was sich jedoch noch etwas hinziehen wird. Im Geiste von Open Science möchte ich jedoch bereits einige Quellen vorstellen; zum Thema "Manchester-Liberalismus". Die heute gängige Vorstellung einer Ideologie, die nur auf das Eigeninteresse und die Vorteile der "Reichen" bedacht war und des Staates als Hüter des Gemeinwohls und Beschützer der Armen, der sich im 19. Jahrhundert völlig aus der Wirtschaft heraushielt, wird in den angeführten Texten als nicht ganz korrekt präsentiert. Vielmehr zeigt sich, dass die Interventionen des Staats die Lage der Armen verschlechterte und durch das Zurückdrängen des Staates verbessert wurde. Ich werde darauf hoffentlich später noch genauer eingehen können. Gerecht ist nur die Freiheit von Richard Herzinger und Mythos Manchestertum von Detmar Doering
7 Kommentare
Mehr anzeigen

I Want To Hold Your Hand

Ich habe gerade im Radio gehört, dass genau heute vor 60 Jahren die Beatles ihr Stück I Want To Hold Your Hand veröffentlicht haben. Dafür unterbreche ich gerne meine Blog-Abstinenz und zeige hier meine Lieblingsinterpretation dieses Lieds aus der wunderbaren Serie GLEE:
3 Kommentare
Mehr anzeigen