Lesbarkeit vs. Abhängigkeiten vermeiden

[Leba]

Hallo,

Robert C. Martin hat in seinem Buch Agile Principles, Patterns and Practices in C# ein einfaches Beispiel für das Refactoring eines Algorithmus aufgeführt.

Es handelt sich um das Sieb des Eratosthenes. Das Ergebnis sieht so aus: PrimeGeneratorByMartin.cs. (die zugehörigen Tests sind hier: PrimeGeneratorTests)

Ich habe dies Beispiel zur Übung einmal noch weiter refactored, so dass es keine Abhängigkeiten in Logik Containern (Methoden) mehr gibt.(also nach IOSP)

Ich muss dazu sagen, dass ich den Datenfluss bzw. den Flow nicht neu entworfen habe, sondern ich habe einfach den vorhandenen Code solange angepasst, bis alle Abhängigkeiten aufgelöst waren. Das Ergebnis sieht so aus: PrimeGenerator.cs.

Eine alternative Integrationsmethode sähe so aus: GeneratePrimeNumbers(). Ich bin mir nicht sicher, was mir besser gefällt.

Die Methode IterateUpTo() könnte man ggf. noch durch Enumerable.Range(...).ToList().ForEach()... ersetzen.

Mir gefällt das Endresultat einerseits sehr gut, da ich die Abhängigkeiten eliminieren konnte.

Andererseits ist das Ergebnis viel schlechter zu lesen, finde ich.

Geht hier das IOSP zu weit? 

Welche Alternativen gibt es?

Sollte man immer zuerst ein Flow Design machen? Allerdings kann ich ja auch nicht immer, wenn ich etwas refactore, einen neuen Entwurf machen, denn das ist ja dann eher Neu-Implementierung.

Oder braucht man bei sehr mathematische Algorithmen ggf. nicht so viel Wert auf das Minimieren von Abhängigkeiten zu legen, solange die "privat" sind? (Denn hier wird es ja wohl sehr selten Abhängigkeiten zu anderen Modulen geben, außer eben zu den privaten Methoden innerhalb der Klasse.)

Der eine Entwurf favorisiert Lesbarkeit, der andere Vermeidung von Abhängigkeiten.

Kann man nicht beides miteinander vereinbaren?

[Tim Gesekus]

Hi,

Onkel Bob mach in seinen Code Beispiel viele Dinge, die die Lesbarkeit erhöhen, die ich in deinem Beispiel nicht gemacht wurden. Nimm mal eine Zeile wie:

                   
 IterateUpTo(limit,i => CheckIfCrossed(crossedOut, i, () => CrossOutputMultiplesOf(crossedOut, i)));

Wenn ich den Algorithmus nicht kenne muss ich jetzt erst mal Analyse machen, was diese Zeile macht:
1. IterateUpTo .. wird wohl bis zum limit hochzaehlen und CheckIf Crossed fuer jedes i machen
2. CheckIfCrossed .. wird wohl .....
3. CrossOutMulitplesOf .. wird wohl .....

und zusammen macht das wohl ....

Viel Denken und Lesen für eine Zeile und der Parameter crossedout kommt auch noch mehrfach darin vor.
Also packe die Zeile in einen Methoden Aufruf:

CrossOutMulitplesOf(..);

Dann hat man wenigstens eine Idee was die Zeile leisten soll.

Ich empfehle das Buch Clean Code :)
Ansonsten gilt für mich Lesbarkeit als eine der wichtigsten Merkmale von Software, allerdings bezogen auf das gewählte Paradigma.

Vergleiche:

UncrossIntegersUpTo(maxValue);

CrossOutMultiples();

PutUncrossedIntegersIntoResult();

mit

IterateUpTo(limit,

i => CheckIfCrossed(crossedOut, i, () => CrossOutputMultiplesOf(crossedOut, i)));

IterateUpTo(crossedOut.Length - 1,

i => CheckIfCrossed(crossedOut, i, () => result.Add(i)));

Das erste ist Prosa und sagt mir was der Code macht und das zweite sagt mir nur wie es etwas macht. Auf der höchsten Abstraktion Ebene will ich aber wissen, was er macht.

HTH

Tim

[Stephan Roth]

Guten Tag,

ich schließe mich Tims Ausführungen an. Das für mich wesentliche Prinzip, welches hier berücksichtigt werden sollte, lautet "Use Intention-revealing names".

Die herausgegriffene Codezeile zeigt zwar, WIE etwas gelöst wurde, es wird aber in keinster Weise ersichtlich WAS dieses Stück Code aus einer fachlichen Perspektive macht.

Versuche einen Semantik-reichen Namen für die Codezeile zu finden, der beschreibt, was sich fachlich dahinter verbirgt.

LG,
Stephan

> --
> Sie erhalten diese Nachricht, weil Sie in Google Groups E-Mails von der Gruppe "Clean Code Developer" abonniert haben.
> Wenn Sie sich von dieser Gruppe abmelden und keine E-Mails mehr von dieser Gruppe erhalten möchten, senden Sie eine E-Mail an clean-code-devel...@googlegroups.com.
> Wenn Sie in dieser Gruppe einen Beitrag posten möchten, senden Sie eine E-Mail an clean-code...@googlegroups.com.
> Gruppe besuchen: http://groups.google.com/group/clean-code-developer
> Weitere Optionen finden Sie unter https://groups.google.com/d/optout.

[Leba]

Dass mein Code schlecht lesbar ist, das habe ich ja bereits selbst erwähnt.

Trotzdem Danke nochmal für die präzisen Erläuterungen. :-)

Ich habe dieses Refactoring als reine Übung gesehen, mit dem Ziel die Abhängigkeiten auf elementarster Ebene also auf Methodenebene nach dem Integration Operation Segregation Principle (IOSP) zu minimieren.

Dazu empfehle ich übrigens mal die Lektüre von Ralf Westphals Blog: http://geekswithblogs.net/thearchitectsnapkin/Default.aspx.

Ich frage mich halt nun WARUM ist das schiefgegangen?

Macht IOSP auf Methodenebene vielleicht nicht immer Sinn?

Habe ich es falsch gemacht? (Ich finde z.B., dass man die Namen der Methoden noch sprechender machen könnte. Aber im Gesamten ändert das nicht so viel an der Lesbarkeit und das Problem bleibt bestehen.)

Und die anderen Fragen, die ich oben gestellt habe.

Wenn ich eure Antworten einmal dahingehend interpretiere, verstehe ich sie so, dass IOSP also nicht immer Sinn macht auf elementarster Ebene, dass Lesbarkeit wichtiger ist als das Minimieren von Abhängigkeiten und es gibt anscheinend aus eurer Sicht keine Alternativen. (Denn sprechendere Namen helfen auch nicht wirklich.) Folglich ist Lesbarkeit und das Minimieren von Abhängigkeiten, was ja beides der z.B. Änderbarkeit des Codes dient, nicht immer vereinbar.

> Wenn Sie sich von dieser Gruppe abmelden und keine E-Mails mehr von dieser Gruppe erhalten möchten, senden Sie eine E-Mail an clean-code-developer+unsub...@googlegroups.com.

[Ralf Westphal]

Ich sehe da keinen Widerspruch zwischen Lesbarkeit und "refactoring to IOSP/PoMO" (also Flow Design).

Hier meine Version der Primzahlengenerierung: https://gist.github.com/ralfw/08dc32b3f51236ad557e

Der Trick besteht darin, denke ich, dass man nicht sklavisch refaktorisieren darf. Man muss eine Balance finden zwischen "Lösungsteile übernehmen und refaktorisieren" und "Neu schreiben" :-)

Meine Lösung enthält daher nur wenig Originalcode von dir bzw. Bob Martin. Der Grund: der Lösungsansatz (!) hat sich mir nicht klar genug erschlossen. Lesbarkeit braucht nicht nur eine gewisse Form (IOSP/PoMO) und gute Namen, sondern darunter einen verständlichen Ansatz.

Deshalb habe ich zweierlei an meiner Lösung verändert, statt nur zu refaktorisieren:

1. Den Test auf den Edge-Case (if (maxValue<2)...) habe ich weggelassen. Der ist überflüssig. Er ist ein Relikt des Vorgehens nach TDD. Da war mal ein Testfall, der so grün gemacht werden konnte. Doch das Wissen, dass es unterhalt 2 keine Primzahlen gibt, ist dadurch an mindestens zwei Stellen codiert. Einmal in dem Vergleich und einmal in PutUncrossedIntegersIntoResult. Dort steht nämlich for(..., i=2; ...). Dadurch werden Zahlen <2 als Primzahlen ebenfalls ausgeschlossen.

Ich habe also diesen Vergleich gestrichen - das ist wirklich KISS, finde ich :-) - und habe die Behandlung von zu kleinen Zahlen über eine Konstante sichtbar gemacht.

2. Das Wegstreichen crossedOut habe ich umgekehrt. Bei Martin werden in dem Feld nicht-Primzahlen auf true gesetzt. Das ist durch die Brust ins Auge. Bei mir heißt das vergleichbare Array prime_candidates. Das ist sozusagen positiv formuliert. Zu Anfang sind alle Zahlen mögliche Primzahlen - und dann werden welche weggestrichen, also auf false gesetzt. Das finde ich natürlicher/verständlicher.

3. Die globalen Variablen bei Martin sind nun wirklich völlig unnötig. Was ihn da geritten hat... [Kopfschütteln]. Bei mir sind die prime_candidates zwar im Fluss - allerdings werden sie noch in place verändert. Ich mache mir die mutability von Arrays zunutze. Das Flow Design ist also nicht streng Functional Programming - muss ja aber auch nicht ;-) Der Seiteneffekt hier ist überschaubar. Kalkuliertes Risiko sozusagen :-)

Daraus ließe sich übrigens auch eine Datenstruktur (ADT) ableiten. Deshalb sind die beiden Methoden Initialize_prime_candidates und Extract_primes_from_candidates etwas abgesetzt am Ende. Sie könnten in einer Klasse Primescandidates oder so zusammengefasst werden. Damit wäre die Trennung von Domänenlogik (Operationen in Zeilen 19 bis 38) und Daten deutlich. Das habe ich hier mal versuchsweise gebaut: https://gist.github.com/ralfw/08dc32b3f51236ad557e#file-primegenerator_with_adt-cs.

Für einen solchen ADT spricht, dass nun die Kenntnis über die kleinste Primzahl (Konstante) in der ADT-Klasse gekapselt ist. Vorher wurde sie einmal in den Methoden rund um prime_candidates benutzt und einmal in den Methoden, die ich zur Kernlogik gezählt hatte. Beim Rausziehen in eine Klasse wurde deutlich, dass da die Kohäsion falsch gesehen war.

Einziges Problem bei der Lösung mit ADT ist, dass die Datenstruktur, die sich selbst enummeriert währenddessen verändert wird. Wenn die Enummeration "außen" stattfindet wie vorher, finde ich das nicht so schlimm. Hm... aber vielleicht ist das eine lässliche Sünde ;-)

An der Erkenntnis, dass IOSP/PoMO einer besseren Lesbarkeit nicht im Wege stehen, ändert das aber nichts. Refactoring ist auch mit IOSP/PoMO nur zu einem gewissen grad mechanisch. Wenn es hakt, dann muss man sich Gedanken machen. Dann ist das ein Hinweis darauf, dass irgendwas noch nicht  stimmt, eine Prämisse hinterfragt werden sollte.

In diesem Fall war schon die Vorlage, sorry to say, nicht so optimal, wie sie schien.

[Leba]

Hallo Ralf,

ich bin froh, dass ich dies kleine Problem gepostet habe, denn ich habe nun viel daraus gelernt (s.u.).

Deine Lösung ist wirklich sehr schön! :-)

Was hältst du von der folgenden kleine Änderung (Stichwort: Mutability von Arrays)?:

Ich finde, dies macht deutlich:

1. Das Array prime_candidates wird verändert 

(es wäre ja auch möglich im Sinne von FP ein neues Object zu erzeugen, aber vielleicht ist das in C# nicht besonders performant bei steigendem MaxValue) 

2. Den Datenfluß (der State ist nicht mehr versteckt)

1. Den Test auf den Edge-Case (if (maxValue<2)...) habe ich weggelassen. Der ist überflüssig. Er ist ein Relikt des Vorgehens nach TDD. Da war mal ein Testfall, der so grün gemacht werden konnte. Doch das Wissen, dass es unterhalt 2 keine Primzahlen gibt, ist dadurch an mindestens zwei Stellen codiert. Einmal in dem Vergleich und einmal in PutUncrossedIntegersIntoResult. Dort steht nämlich for(..., i=2; ...). Dadurch werden Zahlen <2 als Primzahlen ebenfalls ausgeschlossen.

Ich habe also diesen Vergleich gestrichen - das ist wirklich KISS, finde ich :-) - und habe die Behandlung von zu kleinen Zahlen über eine Konstante sichtbar gemacht.

Ja eigentlich ist es sogar DRY, denn das doppelte codieren des Falls max < 2 ist ja redundant.

 

Daraus ließe sich übrigens auch eine Datenstruktur (ADT) ableiten. Deshalb sind die beiden Methoden Initialize_prime_candidates und Extract_primes_from_candidates etwas abgesetzt am Ende. Sie könnten in einer Klasse Primescandidates oder so zusammengefasst werden. Damit wäre die Trennung von Domänenlogik (Operationen in Zeilen 19 bis 38) und Daten deutlich. Das habe ich hier mal versuchsweise gebaut: https://gist.github.com/ralfw/08dc32b3f51236ad557e#file-primegenerator_with_adt-cs.

Den Ansatz mit dem ADT kann ich nachvollziehen, finde ich aber persönlich nicht so gut.

Was soll ich mit dem Typ Primecandidates in einem anderen Kontext? Ohne die Domänenlogik?

Ich finde, das ist nicht wirklich sinnvoll. Arrays, Stacks, Queues etc. sind ja auch nicht vom Kontext abhängig. Warum trennen, was ein so hohes Maß an Kohäsion aufweist?

Ich verstehe diesen Ansatz eher so, dass Primecandidates eine Art Message Objekt ist, welches durch den Flow gereicht wird (z.B. im Sinne von Pipes and Filters).

Aber irgendwie fühlt sich die Trennung der logischen Operationen auf zwei Klassen für mich irgendwie merkwürdig an.

An der Erkenntnis, dass IOSP/PoMO einer besseren Lesbarkeit nicht im Wege stehen, ändert das aber nichts. Refactoring ist auch mit IOSP/PoMO nur zu einem gewissen grad mechanisch. Wenn es hakt, dann muss man sich Gedanken machen. Dann ist das ein Hinweis darauf, dass irgendwas noch nicht  stimmt, eine Prämisse hinterfragt werden sollte.

Ich denke, das habe ich hieraus wirklich mitgenommen: mechanisches Refactoring ist nicht immer sinnvoll, auch wenn man eine gute Testabdeckung hat und sich dabei strikt an Prinzipien hält. Selbst denken ist wichtig, so dass man nicht andere Prinzipien dabei verletzt. Dennoch gibt es zum Glück ;-) keinen Widerspruch zwischen IOSP und Lesbarkeit.

Ich nehme aber auch mit, dass es folglich in der Praxis oft nur schwer möglich ist, sinnvolle Refactorings zu machen, selbst bei guter Testabdeckung. Das Mechanische, was schnell geht, führt nicht immer zum gewünschten Ergebnis. Sich Gedanken zu machen, und größere Teile neu zu implementieren, kann zu viel Zeit kosten.

[Ralf Westphal]

Die Einführung von crossed_out_candidates finde ich gut. Ich hatte selbst einen Moment dran gedacht - mich dann aber dagegen entschieden, weil der Scope überschaubar ist und ich irgendwie das Umkopieren des Array gescheut habe.

Aber das ist natürlich Quatsch. Der Datenfluss wäre schon deutlicher gewesen, auch ohne Umkopieren. So wie du es nun gemacht hast. Schöne Verbesserung.

An dem ADT hänge ich nicht. War nur ne Idee, weil mir ins Auge sprange, dass mindestens zwei Methoden quasi als Klammer um das Ausstreichen auf der Datenstruktur arbeiten.

Das Argument "Was soll ich mit dem Typ Primecandidates in einem anderen Kontext?" kann ich allerdings nicht nachvollziehen. Da steckt "Wiederverwendbarkeit" dahinter. Aber die finde ich vernachlässigenswert. Der ADT ist da, weil er etwas kapselt, eine Responsibility repräsentiert im Rahmen dieser einen Domäne. Völlig uninteressant, ob der ADT nochmal woanders benutzt werden kann. Deshalb können seine Methoden auch auf diesen Anwendungsfall zugeschnitten sein. Nur weil es ein ADT ist, baue ich keinen Framework.

Refactorings, die im Grunde so viel wie möglich behalten wollen, lassen sich wirklich nur selten sinnvoll machen. Refactoring bietet eine Chance zum Review, zur Reflexion nicht nur in Bezug auf Struktur, sondern auch in Bezug auf Lösungsansatz. Wenn der bisher funktionierte, aber letztlich schlecht zu verstehen ist... dann ist es an der Zeit, auch das zu verbessern.

--
Sie erhalten diese Nachricht, weil Sie in Google Groups ein Thema der Gruppe "Clean Code Developer" abonniert haben.
Wenn Sie sich von diesem Thema abmelden möchten, rufen Sie https://groups.google.com/d/topic/clean-code-developer/2rkrMmt3u7o/unsubscribe auf.
Wenn Sie sich von dieser Gruppe und allen Themen dieser Gruppe abmelden möchten, senden Sie eine E-Mail an clean-code-devel...@googlegroups.com.

Wenn Sie in dieser Gruppe einen Beitrag posten möchten, senden Sie eine E-Mail an clean-code...@googlegroups.com.
Gruppe besuchen: http://groups.google.com/group/clean-code-developer
Weitere Optionen finden Sie unter https://groups.google.com/d/optout.

--

Ralf Westphal - One Man Think Tank

Hans-Henny-Jahnn-Weg 44
D-22085 Hamburg
Germany
Tel 0170-3200458
Email in...@ralfw.de

[Leba]

Ich habe bei meiner Version ebenso die Prüfung auf MaxValue > 2 eliminiert, 3 Methoden extrahiert und etwas bessere Namen gewählt. (siehe PrimeGenerator.cs)

Das Resultat ist nicht so schlecht wie die erste Version, finde ich, auch wenn Ralfs Version immer noch sauberer ist.

Grüße

Leif

Wenn Sie sich von dieser Gruppe und allen Themen dieser Gruppe abmelden möchten, senden Sie eine E-Mail an clean-code-developer+unsub...@googlegroups.com.

Wenn Sie in dieser Gruppe einen Beitrag posten möchten, senden Sie eine E-Mail an clean-code...@googlegroups.com.
Gruppe besuchen: http://groups.google.com/group/clean-code-developer
Weitere Optionen finden Sie unter https://groups.google.com/d/optout.