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

Labels:

Kommentare

Kommentar veröffentlichen

Beliebte Posts aus diesem Blog

Utopie gesucht

In den 90er Jahren gab es meiner Meinung nach eine positive Zukunftssicht. Das sah man u.a. in der Serie Star Trek The next generation. Heute dagegen scheint es nur noch pessimistische Blicke auf die Zukunft zu geben. Auch die aktuellen Star Trek Serien stellen eine düsterere Welt dar. Dies könnte zu einer selbst erfüllenden Prophezeiung werden. Gibt es in der aktuellen Popkultur noch Utopien?
18 Kommentare
Mehr anzeigen

Avatar - mit Glatze bitte!

Vor einigen Monaten kamen zwei neue Mitarbeiter in unser Team. Da einer von ihnen auch Martin hieß, führte das oft zu Verwechslungen und der Nachfrage „welcher jetzt?“ – vor allem in der Remote-Kommunikation. Beschreibungen wie „der zweite Martin“ oder „der andere Martin“ hielt ich für unpassend. Also schlug ich vor, dass ich von nun an einfach einen anderen Rufnamen erhalte und wählte: „Guybrush“. Damit das auch immer präsent ist, ersetzte ich auch mein Profilbild an allen mir möglichen Orten durch ein Pixelbild von Gybrush aus Monkey Island 2 : Wie erwartet, setzte sich das sehr schnell durch. Vor einigen Tagen wollte ich dieses Bild jedoch durch eine etwas auflösungsstärkere Variante ersetzen. Die gefundenen Bilder machten jedoch sehr deutlich, dass es doch erheblich an Ähnlichkeit mangelte. Vor allem die Haare entsprachen so gar nicht meiner Frisur. Also beschloss ich, dass dies doch mal ein guter Einsatz für einen AI-Bildgenerator wäre. Die zwei mit denen ich bisher gearbeitet h...
3 Kommentare
Mehr anzeigen

Markt und Staat - Teil 1

"Entschuldigen Sie bitte! Was heißt Mittagessen nach Vortragsthema?" "Nun, das heißt, dass der Ablauf des Mittagessens sich nach den Themen des jeweiligen Tages richtet." "Können Sie mir das etwas genauer erklären?" "Aber gerne. Sehe Sie, der erste Tag steht unter dem Thema 'Der demokratische Staat'. Zu Tagesbeginn sammeln wir von allen Konferenzteilnehmern 5,- Euro ein: die Mittagessenpauschale." "Verstehe." "Im Laufe des Vormittages teilen wir Speisekarten mit den verfügbaren Mahlzeiten aus. Sie kreuzen an, welches Gericht Ihnen zusagt und geben die Karte bis zum Mittag wieder bei uns ab." "Ok. Ich schreibe also meinen Namen auf den Zettel..." "Nein." "Aber woher wollen Sie dann wissen für wen welches Gericht ist?" "Das ist nicht wichtig. Alle Teilnehmer bekommen das gleiche." "Aber warum dann die Sache mit dem Ankreuzen?" "Um festzustellen, für welche Mahlzeit s...
10 Kommentare
Mehr anzeigen