Mikroschritte in Code-Kata

December 1, 2009 at 6:22 pm 10 comments


Bernd Schiffer und ich haben eine Code-Kata mit TDD programmiert. Nach ein paar Refactorings hatten wir Groovy-Code, der ungefähr so aussah (ich habe das konkrete Beispiel gegen ein einfacheres ausgetauscht):

def toString(number, range = (1..number).reverse()) {
    if (range.empty) return ""
    def item = range.last()
    "$item" + toString(number, range - item)
}

assert '1' == toString(1)
assert '12' == toString(2)
assert '123' == toString(3)
assert '1234567891011' == toString(11)

"toString" liefert einen String mit den Zahlen 1 bis "number". In "range" finden sich die jeweils noch zu bearbeitenden Zahlen. Das kann man in Groovy natürlich viel einfacher hinschreiben:

def toString(number) {
	(1..number).join() 
}


Aber für den Punkt dieses Blog-Beitrags bleiben wir bei der ursprünglichen Implementation. Wir bleiben also bei der "range" und fokussieren auf ein anderes Problem. Wir verdrehen bei der Erzeugung der Range die Reihenfolge mit "reverse" und greifen hinterher mit "last" darauf zu. Es wäre einfacher, wenn wir die Reihenfolge unverändert ließen und mit "first" auf das jeweils erste Element zugreifen würden. Wir brauchen also ein Refactoring, das diese beiden Stellen im Code ändert.

Dummerweise steht der Bernd davor. Er ist dagegen, dass wir einfach diese zwei Stellen im Code ändern. Er meint, das sei nicht der "grüne Weg", weil nach der ersten Änderung (entfernen von "reverse") die Tests (asserts) nicht mehr laufen. "Aber alle zwei Änderungen finden in derselben Methode statt. Das kann man doch problemlos in einem Schritt ändern und dann ist alles grün. So ist es doch viel schneller." Aber Bernd bleibt dabei. Das ist ihm zu großschrittig (ein Schulungsteilnehmer sagte einmal sinngemäß "Ich dachte immer kleiner als Elektron geht nicht, aber jetzt kenne ich den Bernd-Schritt als kleinstes Element."):

  1. Wir arbeiten an einer Code-Kata. Dabei geht es darum, dass wir unsere Programmierfähigkeiten verbessern. Es geht nicht darum, die Kata in möglichst kurzer Zeit durchzuführen. Es geht darum, sie möglichst gut durchzuführen - wie bei einer Karate-Kata.
  2. Wir stehen in der Praxis immer wieder vor großen Refactorings, die wir nicht in kleine Schritte zerlegt bekommen und die uns den letzten Nerv kosten. Je kleinschrittiger wir vorgehen können, umso besser können wir große Refactorings zerlegen. Wenn wir es schaffen, dass die Tests nach jeder Änderung eines Ausdrucks grün sind, dann können wir für jedes große Refactoring einen komplett grünen und damit sicheren Weg beschreiten.
  3. Wenn wir mehrere Änderungen auf einmal durchführen, laufen wir immer Gefahr, versehentlich Funktionalität zu erweitern - die dann nicht durch Tests abgedeckt ist. Wenn wir jeweils nur einen Ausdruck ändern, können wir normalerweise sehr sicher abschätzen, ob wir die Funktionalität erweitert haben.

"OK, Bernd hat Recht. Versuchen wir es Kleinstschrittig. Äh, geht das denn überhaupt?"

Ja, es geht. Gleicht kommt die Lösung. Wer sich selbst an der Aufgabe versuchen möchte, sollte hier nicht weiterlesen und es erstmal selbst versuchen. Die Regel: Einen Ausdruck ändern, Tests ausführen, es muss alles grün sein. Danach der nächste Ausdruck.

Eine Lösung
Es gibt verschiedene Möglichkeiten, das Problem zu lösen. Hier eine mögliche Lösung:

Zuerst vereinfache ich den "range"-Defaultwert, indem ich das umdrehen der Elemente bereits beim Hinschreiben erledige:

def toString(number, range = number..1) {
    if (range.empty) return ""
    def item = range.last()
    "$item" + toString(number, range - item)
}


Jetzt kommt der interessante Teil. Die Reihenfolge der Elemente in "range" hängt direkt mit dem Zugriff über "last" zusammen. Wenn ich "last" durch "first" ersetzen möchte, muss ich dafür sorgen, dass in "range" am Anfang und Ende dasselbe Element steht. Das erreiche ich, indem ich "range" quasi verdoppele:

def toString(number, range = (1..number)+(number..1)) {
    if (range.empty) return ""
    def item = range.last()
    "$item" + toString(number, range - item)
}


Dass "range" jetzt jede Zahl doppelt enthält, schadet nicht, weil "range - item" alle "item"s entfernt. Mit diesem verdoppelten Range liefert "last" dasselbe Ergebnis wie "first", so dass ich die Ersetzung problemlos vornehmen kann.

def toString(number, range = (1..number)+(number..1)) {
    if (range.empty) return ""
    def item = range.first()
    "$item" + toString(number, range - item)
}


Jetzt kann ich den Default-Wert für "range" wieder vereinfachen:

def toString(number, range = 1..number) {
    if (range.empty) return ""
    def item = range.first()
    "$item" + toString(number, range - item)
}


Und fertig bin ich mit meinem Refactoring. Wir haben nach jeder Änderung eines Ausdrucks die Tests durchlaufen lassen und sie waren immer grün. In der Praxis würde man in diesem Beispiel vielleicht nicht so kleinschrittig vorgehen. Aber hier geht es um den Trainingseffekt!

Eine interessante Frage ist sicherlich, ob es immer solche kleinschrittigen Auflösungen gibt oder ob es Fälle gibt, wo das nicht funktioniert. Wenn Ihr Refactoring-Beispiele habt, von denen Ihr glaubt, dass es keine Kleinstschrittige Zerlegung gibt, schickt mir die! Entweder an stefan AT stefanroock DOT de (AT durch @ und DOT durch . ersetzen) oder als Kommentar an diesen Blog-Beitrag.

Entry filed under: #. Tags: , .

Programmierkatas Vortrag zum inkrementellen Entwurf auf XP-Days und Scrum-Gathering

10 Comments Add your own

  • 1. Johannes Link  |  December 2, 2009 at 8:49 am

    Der gezeigte Lösungsweg scheint mir ein verschleiertes Beispiel für ein gängiges Refactoring-Pattern zu sein: Zuerst die neue Struktur parallel aufbauen, bis sie funktioniert, und dann erst die alte rauswerfen. Mit diesem Gedanken kommt man dann auch gleich auf einen nicht ganz so clevere Lösung, die mir aber wesentlich verständlicher scheint:
    1. Zusätzlichen Parameter “unreversedRange” mit default (1..number) einführen
    2. Zugriff in der Methode Schritt für Schritt darauf umlenken
    3. Dann den ursprünglichen Parameter rauswerfen
    4. unreversedRange nach range umbenennen
    5. fertig mit deutlich weniger Cleverness

  • 2. stefanroock  |  December 2, 2009 at 9:01 am

    Du hast Recht. Allerdings ist “Methodenparameter rauswerfen” ohne IDE-Support für sich genommen schon mehrschrittig.

  • 3. Johannes Link  |  December 2, 2009 at 9:14 am

    @Stefan “Allerdings ist „Methodenparameter rauswerfen“ ohne IDE-Support für sich genommen schon mehrschrittig.”
    Nein, wenn es der letzte Parameter ist (du hast die Wahl!) und er einen Default-Wert hat, kannst du das Stelle für Stelle machen.

    Ohne IDE-Unterstützung ist Rename übrigens hochgradig mehrschrittig!

  • 4. Stefan Roock  |  December 2, 2009 at 9:28 am

    >Nein, wenn es der letzte Parameter ist (du hast die Wahl!) und er
    >einen Default-Wert hat, kannst du das Stelle für Stelle machen.
    Stimmt, Groovy sei dank🙂 Hatte zu sehr Java gedacht.

  • 5. Stefan Roock  |  December 2, 2009 at 9:29 am

    >Nein, wenn es der letzte Parameter ist (du hast die Wahl!) und er
    >einen Default-Wert hat, kannst du das Stelle für Stelle machen.
    Dann kann man gegen Deine Lösung eigentlich nur einwenden, dass sie langweilig ist🙂

  • 6. Bernd Schiffer  |  December 2, 2009 at 12:00 pm

    Ich stelle mich wieder davor, diesmal vor Johannes: Seine Lösung ist von den Schritten her wesentlich aufwändiger als Stefans, wenn er die automatisierten Refactorings beiseite läßt. Zum Vergleich:

    Stefan braucht die Schritte:
    – Umstellen von (1..number).reverse() auf (number..1)
    – Hinzufügen von (1..number)+
    – Umstellen von range.last() auf range.first()
    – Entfernen von +(number..1)
    Summe: 4

    Johannes braucht die Schritte:
    – Einfügen eines Default-Parameters , unreversedRange = 1..number
    – Benutzen des neuen Parameters im rekursiven Methodenaufruf , unreversedRange – item
    – Bedingung für Rekursionsabbruch anpassen an neuen Parameter via || unreversedRange.empty
    – Umstellen von range.last() auf unreversedRange.first()
    – Umstellen des Default-Parameters von range = (1..number).reverse() auf range = number..1
    – Umstellen von unreversedRange.first() auf range.first()
    – Löschen der Bedingung || unreversedRange.empty
    – Löschen des Parameters im rekursiven Methodenaufruf , unreversedRange – item
    – Löschen des Default-Parameters , unreversedRange = 1..number
    Summe: 9

    Johannes’ Lösung sieht mir nicht wesentlich verständlicher aus als Stefans. Auch, wenn ich Rename als atomar ansehen würde (tue ich nicht mehr und das ist eine schöne Erkenntnis für mich), so bräuchte Johannes 5 Schritte.

    Sehe deutliche Parallelen zu DRY: DRY-Code muss nicht unbedingt lesbarer sein als solcher, der Redundanzen enthält. Analog dazu: der kürzeste grüne Pfad ist nicht unbedingt verständlicher als ein längerer.

    Bei DRY würde ich die unleserlichere aber redundanzfreie Struktur vorziehen vor einer leserlichereren mit Redundanzen, weil sie aus meiner Sicht wartbarer ist. Analog dazu würde ich den kürzeren aber unverständlicheren grünen Pfad vorziehen vor einem längeren aber verständlicheren, weil er schneller beschreitbar ist (= effizienteres Refactoring).

  • 7. Johannes Link  |  December 2, 2009 at 12:23 pm

    @Bernd
    Unlesbarer DRY-Code ist mMn nicht viel wert, weil er mein Design-Prinzip verletzt, dass der Code für den Kunden gschrieben sein muss. Genau so sehe ich das auch im Refactoring: Stell dir vor, du müsstest bei deinem kürzeren Weg nach Schritt 2 (1..number)+ das Projekt verlassen. Wer sollte verstehen, auf welchem Weg du gerade warst? Die kleinen Schritte mache ich ja auch, damit ich das Refactoring für unbestimmte Zeit unterbrechen kann. Meist nur für einen Update/Commit-Zyklus, manchmal aber auch bis zur nächsten Iteration oder bis auf unbestimmt.

    Daher mein Fazit: Mehr, aber dafür kleinere Schritte finde ich besser als ein auf Teufel komm raus optimierter Refactoring-Weg, den ich nicht guten Gewissens unterbrechen kann.

  • 8. Bernd Schiffer  |  December 2, 2009 at 5:44 pm

    @Johannes
    Habe nie etwas von “unlesbarem” Code geschrieben. Habe von “unlesbarerem” (Komparativ) Code geschrieben.

    Meine höhere Prio liegt auf DRY denn auf mehr (!) Lesbarkeit. Sehe nicht, wo unlesbarerer DRY-Code nicht viel wert sein soll gegenüber lesbarerem unDRY-Code. Ist aus meiner Sicht objektiv nicht entscheidbar.

    > Daher mein Fazit: Mehr, aber dafür kleinere Schritte finde
    > ich besser als ein auf Teufel komm raus optimierter
    > Refactoring-Weg, den ich nicht guten Gewissens
    > unterbrechen kann.

    Sehe nicht, wo Deine Schritte kleiner sind als Stefans. Dein Weg ist länger, daher mehr Schritte nötig.

    Wage zu behaupten, dass jedes nicht zu Ende durchgeführte Refactoring ohne entsprechende Kommunikation ein Risiko hat oder ein schlechtes Gewissen hinterlässt.

    Bei großen Refactorings stellt man typischerweise “Baustellenschilder” (aka Kommentare, Meetingsverkündigungen, Rund-E-Mails usw.) auf, damit jeder im Team und der Kunde Bescheid weiss. Das hat Stefan nicht getan. Aber Du auch nicht. Wenn bei Deinen mehreren Schritten mittendrin abgebrochen würde, dann würde der Kunde doch auch verwirrt sein, wenn er zwei default-paramter range und unreversedRange in der Methodensignatur findet.

    Himmel, worüber schreiben wir hier überhaupt? “Stell dir vor, du müsstest bei deinem kürzeren Weg … das Projekt verlassen.” Dann darf mein Kunde gerne meine paar Sekunden Arbeit wegschmeissen. Oder das Rollback veranlassen, wenn ich beim Abgang den Commit-Button erwischt haben sollte😉

  • 9. Marko Schulz  |  December 2, 2009 at 11:04 pm

    Wie Johannes beschreibt, ist in beiden Lösungen das übergreifende Refactoring-Pattern “Parallelstruktur aufbauen”.

    Stefans Lösung benutzt einen kleveren Kniff, wodurch sie wesentlich kürzer ausfällt, aber das Pattern auch weniger klar erkennbar ist.

    Ich habe Ranges in Groovy so selten benutzt, dass mir vor dem Ausprobieren gar nicht klar war, dass der Kniff von Stefan funktioniert.

    Deswegen lief mein eigener Lösungsversuch ähnlich ab wie der von Johannes beschriebene Weg; allerdings kam ich nach Aufbau der Parallelstruktur nicht weiter, als ich die Ursprungs-Range atomar entfernen wollte. Auf die von Bernd später genannte Möglichkeit, die Ursprungs-Range dann auf “unreverse” umzustellen, bin ich nicht gekommen.

    Generell machen es einem die quasi durchnummerierten, nach ihrer Position unterschiedenen Methodenparameter schwerer, Parallelstrukturen auf- und abzubauen. Benannte Methodenparameter machen dies z.B. leichter. Eine komplett neue, zweite Methode wäre eine andere Möglichkeit.

    Übrigens: Ob das ganze überhaupt ein Refactoring ist, hängt von der Definition ab. Wenn man die Asserts als Spezifikation ansieht, dann ist es ein Refactoring. Wenn man sagt, dass die Rückgabewerte der Methode für alle möglichen Parameter gleich bleiben soll, dann gilt dies nicht mehr: Denn dann haben die ursprüngliche und die endgültige Methode unterschiedliche Ergebnisse für den Aufruf toString(1, (1..5))

  • 10. Johannes Link  |  December 3, 2009 at 11:30 am

    @Marko Zum Thema “Ob das ganze überhaupt ein Refactoring ist”:

    Um bestimmen zu können, ob das “observable behavior” (Martin Fowler) gleich bleibt oder nicht, muss man sich auf einen “observer” einigen. Für mich sind das in den allermeisten Fällen die Tests (bzw. eine festzulegende Untermenge der Tests). Alles, was von diesen Tests nicht erfasst wird, ist zufälliges Verhalten, das man verändern darf. Andernfalls wären eigentlich alle Änderungen, die zur Ausführung von anderen Instruktionen auf dem Prozessor führen als der vorige Code, keine Refactorings, denn man KÖNNTE sie beobachten, wenn man nur genau genug hinschaute.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Trackback this post  |  Subscribe to the comments via RSS Feed



%d bloggers like this: