Redundante Variablen

[temi]

IntelliJ meldet an dieser Stelle eine redundante Variable:

String spacesRemoved = methodToInvoke.replaceAll("\\s+","");
this.methodToInvoke = spacesRemoved;

Das ist auch klar, denn ich könnte einfach schreiben:

// remove spaces
this.methodToInvoke = methodToInvoke.replaceAll("\\s+","");

Ich finde die erste Variante allerdings deutlich besser lesbar, weil sie implizit aussagt, was passiert, während bei der zweiten Variante ein Kommentar angebracht ist.
Gibt es einen Grund (Laufzeit?) der für die zweite Variante spricht oder wird das sowieso identisch übersetzt?
Mir ist schon klar, das der Laufzeitunterschied (wenn überhaupt) minimal wäre, aber ich hatte mal in C++ reingeschnuppert und da macht man sich intensive Gedanken, ob eine Variable kopiert oder gemoved wird.

 

[mrBrown]

Ich finde die erste Variante allerdings deutlich besser lesbar, weil sie implizit aussagt, was passiert, während bei der zweiten Variante ein Kommentar angebracht ist.

Da streiten sich die Geister
Ich würde ein this.methodToInvoke = removeSpaces(methodToInvoke); bevorzugt.

Gibt es einen Grund (Laufzeit?) der für die zweite Variante spricht oder wird das sowieso identisch übersetzt?
Mir ist schon klar, das der Laufzeitunterschied (wenn überhaupt) minimal wäre, aber ich hatte mal in C++ reingeschnuppert und da macht man sich intensive Gedanken, ob eine Variable kopiert oder gemoved wird.

Das wird keinen Unterschied in der Laufzeit machen, der JIT macht da eh draus, was er will

 

[temi]

Da streiten sich die Geister
Ich würde ein this.methodToInvoke = removeSpaces(methodToInvoke); bevorzugt.

Das wäre natürlich am Besten, aber für diesen Einzelfall eine separate Methode spendieren, finde ich etwas übertrieben. Vor allem, weil es sich auch so nur um einen Einzeiler handelt. Bliebe allenfalls eine externe Bibliothek, die diese Funktion anbietet, aber das fände ich für eine einzelne Anwendung auch zu viel des Guten.
Bei der Laufzeit hätte ich so etwas ähnliches vermutet.
Danke,
temi

 

[mrBrown]

Warum ist dafür eine Methode übertrieben?

Wie du doch selbst sagst, das wäre am besten

Man verliert mit der Methode nichts - gewinnt aber Lesbarkeit und Testbarkeit, und gibt es wichtigeres als die beiden?

 

[temi]

Man verliert mit der Methode nichts - ...

Bedeutet übersetzt: Aufgrund der Kürze, wird der Compiler das vermutlich inline machen wodurch es sich von der Laufzeit verhält als wäre es direkt hingeschrieben?
Lässt sich das Inlining irgendwie beeinflussen oder muss man vermuten, dass es so sein wird?

... und gibt es wichtigeres als die beiden?

Ich denke das Laufzeit an kritischen Stellen schon in Betracht gezogen werden muss, aber ich habe leider keinerlei Vorstellung davon, welche Optimierungen in dieser Hinsicht bei Java auch wirklich sinnvoll sind.

 

[mrBrown]

Bedeutet übersetzt: Aufgrund der Kürze, wird der Compiler das vermutlich inline machen wodurch es sich von der Laufzeit verhält als wäre es direkt hingeschrieben?
Lässt sich das Inlining irgendwie beeinflussen oder muss man vermuten, dass es so sein wird?


Ich denke das Laufzeit an kritischen Stellen schon in Betracht gezogen werden muss, aber ich habe leider keinerlei Vorstellung davon, welche Optimierungen in dieser Hinsicht bei Java auch wirklich sinnvoll sind.

Wenn der Overhead von Methodenaufrufe relevant ist, sollte man nicht Java, sondern besser handgeschriebenen Assembler nutzen
Generell ist es Unsinn, bei sowas kleinem an die Laufzeit zu denken - methodenaufrufe haben im Vergleich zu allem anderem so wenig Overhead, da gibt es ganz andere Punkte, grad wenn man Reflection nutzt. Da sollte Lesbarkeit in jedem Fall vorgehen.

Beeinflussen kann man das nicht, kann auch mit ner anderen JVM wieder völlig anders optimiert werden

 

[stg]

Rules of Optimization:
1. Don't.
2. Don't yet.

 

[mrBrown]

Rules of Optimization:
1. Don't.
2. Don't yet.

Die zweite gilt aber nur für Experten ;p

 

[JStein52]

Rules of Optimization:
1. Don't.
2. Don't yet.

Das war ja nicht die Frage, sondern die lautete ursprünglich:
- warum denn einfach wenns auch umständlich geht

 

[mrBrown]

Findet wirklich jemand

String spacesRemoved = methodToInvoke.replaceAll("\\s+","");
this.methodToInvoke = spacesRemoved;

einfacher (zu lesen/schreiben/testen) als

this.methodToInvoke = removeSpaces(methodToInvoke);

?

 

[JStein52]

Nein, aber :

this.methodToInvoke = methodToInvoke.replaceAll("\\s+","");

 

[mrBrown]

Du findest ernsthaft ein methodToInvoke.replaceAll("\\s+",""); schöner als ein ein removeSpaces(methodToInvoke);?

 

[JStein52]

ich finde es nicht schöner sondern genau so schön.

Edit ... und ich finde

 "Halli " + " Hallo"

genau so schön wie

haengeStringsZusammen("Halli ", "Hallo");

 

[Meniskusschaden]

Du findest ernsthaft ein methodToInvoke.replaceAll("\\s+",""); schöner als ein ein removeSpaces(methodToInvoke);?

Ich stimme @mrBrown zu und finde die zweite Variante deutlich besser lesbar (obwohl ich aus Bequemlichkeit trotzdem auch mal zur ersten greifen würde). Bei der ersten Variante werden zwei Abstraktionsebenen vermischt. Für das Verständnis des eigentlichen Algorithmus ist nur wichtig, dass die Leerzeichen entfernt werden. Es ist nicht relevant, wie das geschieht. Die erste Variante kann man nur verstehen, wenn man weiß, was der reguläre Ausdruck bewirkt. In der zweiten Variante ist das nicht nötig.

 

[mrBrown]

Edit ... und ich finde

 "Halli " + " Hallo"

genau so schön wie

haengeStringsZusammen("Halli ", "Hallo");

Das ist auch ein ziemliches Unsinnsbeispiel.
Vergleichbarer wäre da eher ein

String[]  strings;
// strings mit 0 bis n Strings füllen
String string = String.join(" ", strings);

vs

String[]  strings;
// strings mit 0 bis n Strings füllen
String string = "";
// irgendeine hässliche Schleife und ifs um die Strings zu konkatenieren 
// und für die passende Menge an Leerzeichen zu sorgen...

 

[JStein52]

Das ist auch ein ziemliches Unsinnsbeispiel.

Nein, ist genau das was du auch machst : Du machst um einen Methodenaufruf (removeAll(...) ) eine weitere Methode die
nichts tut ausser die andere aufrufen. Und wenn er über kurz oder lang drauf kommt dass er nicht nur Spaces sondern auch noch irgendwelche Sonderzeichen entfernen will ist sogar der Name dieser Methode noch falsch. Also auch noch wartungsunfreundlich.

 

[mrBrown]

Nein, ist genau das was du auch machst : Du machst um einen Methodenaufruf (removeAll(...) ) eine weitere Methode die
nichts tut ausser die andere aufrufen.

Ja, um der Operation einen sinnvollen Namen zu geben und das wie zu kapseln.
Beides ist bei einem + aber schon gegeben - es lässt sich nicht deutlicher schreiben, dass da etwas konkateniert wird, und das wie ist gleichzeitig auch schon gekapselt.

Ein removeSpaces sagt aber deutlicher als ein removeAll + Regex, was da gemacht wird, und versteckt gleichzeitig das wie - ob das mit Regex passiert oder eine schlichte for oder was auch immer ist ziemlich egal.

Und wenn er über kurz oder lang drauf kommt dass er nicht nur Spaces sondern auch noch irgendwelche Sonderzeichen entfernen will ist sogar der Name dieser Methode noch falsch. Also auch noch wartungsunfreundlich.

Wow - du hast erkannt, das der Name nicht mehr passt, wenn die Methode was anderes macht, Glückwunsch.
Sollte removeAll irgendwann mal eine 42 an jeden String hängen, macht sie auch nicht mehr, was der Name sagt.
Sollte irgendwie logisch sein, dass man die Methode auch umbenennt, wenn sie was anderes macht?

Wie testet man eigentlich das entfernen der Leerzeichen mit Regex, so unabhängig von der ganzen anderen validierung?

 

[JStein52]

Wow - du hast erkannt, das der Name nicht mehr passt, wenn die Methode was anderes macht, Glückwunsch.

Du schreibst mal wieder Unsinn. Und man testet das genau so wie man es testet wenn man eine Methode drumrumbastelt, such dir was aus. Ich habe schon hunderte von removeAll(..) gesehen und geschrieben. Auf die Idee eine zusätzliche Methode drumrum zu machen kam noch keiner.

 

[mrBrown]

Du schreibst mal wieder Unsinn.

Was ist daran Unsinn?

Und man testet das genau so wie man es testet wenn man eine Methode drumrumbastelt, such dir was aus.

Also das removeSpaces kann ich einfach so testen ohne drum herum, einfach aufrufen und Ergebnisse vergleichen

Das bei der Variante mit Regex die Leerzeichen passend entfernt werden, kann man nur mit der ganzen Methode drum herum testen, oder man kopiert das, aber C&P ist schon ziemlich dämlich.

Ich habe schon hunderte von removeAll(..) gesehen und geschrieben. Auf die Idee eine zusätzliche Methode drumrum zu machen kam noch keiner.

Keine Ahnung was du so siehst, aber vllt wäre ein bisschen Lektür im Sinne von CleanCode, TDD etc empfehlenswert
Erstaunlicherweise ist so ziemlich jeder, der sich mehr mit Software-Engeneering beschäftigt, der Meinung, das es sinnvoll ist sowas zu kapseln
Das ist alles andere als unüblich, eine Methode in einer anderen zu kapseln, sogar wenn es nur ein Methodenaufruf ohne jegliche Parameter ist.

 

[Flown]

Erstaunlicherweise ist so ziemlich jeder, der sich mehr mit Software-Engeneering beschäftigt, der Meinung, das es sinnvoll ist sowas zu kapseln

Das ist alles andere als unüblich, eine Methode in einer anderen zu kapseln, sogar wenn es nur ein Methodenaufruf ohne jegliche Parameter ist.

Wenn es nur an einer Stelle gebraucht wird, dann ist es sinnlos. Sobald Verhalten geteilt wird - im Sinne von Wiederverwendbarkeit - dann macht es schon Sinn eine Methode drumrum zu basteln (auch für die Testbarkeit).

 

[mrBrown]

Wenn es nur an einer Stelle gebraucht wird, dann ist es sinnlos. Sobald Verhalten geteilt wird - im Sinne von Wiederverwendbarkeit - dann macht es schon Sinn eine Methode drumrum zu basteln (auch für die Testbarkeit).

Seh ich anders - auch wenn es nur an einer Stelle gebraucht wird, kann man dem ganzen einen Sinnvollen Namen geben

Gibt es auch nur einen Nachteil für eine eigene Methode (abseits von „die Klasse wird drei Zeilen länger)?

 

[JStein52]

Das bei der Variante mit Regex die Leerzeichen passend entfernt werden, kann man nur mit der ganzen Methode drum herum testen

Seit wann kann man jetzt regex nicht mehr testen ?

Unsinn kommt aus dem Mittelhochdeutschen, "uhne Sinn". Und das was du da geschrieben hast hat keinen Sinn

 

[JStein52]

Sobald Verhalten geteilt wird - im Sinne von Wiederverwendbarkeit - dann macht es schon Sinn eine Methode drumrum zu basteln

Genau, dann schon

 

[mrBrown]

Seit wann kann man jetzt regex nicht mehr testen ?

Du meinst, den Regex den ich dafür aus der Methode raus kopieren müsste, sodass der Tests nichts mehr mit dem verwendeten Regex zu tun hat?



Da gibt es so ein wunderschönes Beispiel von Kent Beck (korrigiert mich wenn ich mich irre) zu Smalltalk.
Da gibt es die Methode highlight, die nichts anderes tut, als auf dem gleichen Objekt reverse aufzurufen.
Der Grund ist weder, das oft genutztes verhalten ausgelagert wird, noch das es testbar wird, noch irgendwas anderess - außer der reinen Benennung, highlight macht die Intention des aufrufenden ganz offensichtlich deutlicher als ein reverse.

Ist das in euren Augen sinnlos und Kent Beck hat keine Ahnung?

 

[Meniskusschaden]

Wenn es nur an einer Stelle gebraucht wird, dann ist es sinnlos.

Wenn die These der besseren Lesbarkeit stimmen sollte, wäre es auch bei einmaligem Gebrauch sinnvoll. Für mich ist es aus zwei Gründen besser lesbar:

1. Bei einem gut gewählten Methodennamen ist der Sinn sofort verständlich. Bei einem regulären Ausdruck muß ich einen Moment nachdenken.
2. Durch den Methodennamen ist der Zweck der Aktion dokumentiert. Ohne den Methodenaufruf weiß ich nur, dass die Leerzeichen entfernt werden. Ich weiß aber nicht, ob das überhaupt die Absicht des Programmieres war oder ein Programmierfehler. Durch die Einführung der Methode hat der Programmierer dokumentiert, was er überhaupt erreichen will und man kann prüfen, ob der Code das wirklich tut.

Da gibt es so ein wunderschönes Beispiel von Kent Beck ...

In Clean Code erwähnt Robert Martin im Abschnitt über Funktionslängen auch ein Beispiel, in dem Kent Beck viele sehr kurze Funktionen verwendet. Er hat es positiv dargestellt, konnte den konkreten Code aber leider nicht bereit stellen.

 

[JStein52]

Folgendes ist Unsinn: eine Regex wie die des TE die spaces entfernt ist trivial. Eine Regex die etwas komplexer ist die entwerfe ich nicht in meinem Hirn, schreibe sie dann in removeAll und dann fange ich an zu testen ob sie das tut was ich will. Die entwerfe ich schrittweise mit einem passenden regex-tester (links dazu gab es hier schon jede Menge, wenn du einen brauchst kannst du ihn gerne kriegen) und kopiere sie dann in mein removeAll (oder was auch immer)

 

[JStein52]

konnte den konkreten Code aber leider nicht bereit stellen.

theoretiker halt.

 

[Flown]

Wenn die These der besseren Lesbarkeit stimmen sollte, wäre es auch bei einmaligem Gebrauch sinnvoll. Für mich ist es aus zwei Gründen besser lesbar:

1. Bei einem gut gewählten Methodennamen ist der Sinn sofort verständlich. Bei einem regulären Ausdruck muß ich einen Moment nachdenken.
2. Durch den Methodennamen ist der Zweck der Aktion dokumentiert. Ohne den Methodenaufruf weiß ich nur, dass die Leerzeichen entfernt werden. Ich weiß aber nicht, ob das überhaupt die Absicht des Programmieres war oder ein Programmierfehler. Durch die Einführung der Methode hat der Programmierer dokumentiert, was er überhaupt erreichen will und man kann prüfen, ob der Code das wirklich tut.

Klar, stimme ich dir zu. Aber in diesem konkretem Beispiel lese ich das genauso wie es programmiert wurde (s replaceAll whitespaces with empty string). Ist jetzt vermutlich viel geschmackssache dabei.
Wenn man deinem Beispiel folgt, kann es sein das auch die Intention "removeSpace" falsch interpretiert werden kann. Will ich jetzt nur " " oder " \t\r\n" entfernen.
Aber das geht jetzt hier auch zu weit und soll keine Rechtfertigung sein. Wenn es jemanden hilft die Lesbarkeit zu erhöhen, dann soll er das bitte auch machen.

 

[Meniskusschaden]

theoretiker halt.

Nein, er hatte ihn einfach nicht. Und Kent Beck laut Martin ebenfalls nicht mehr.
In dem Buch gibt es aber einige andere Beispiele, in denen Kommentare durch kurze Funktionen oder Methoden ersetzt werden.

 

[Flown]

Auch in meinem Studium und in der Arbeit gibt es Leute die gerne Programmieren, wie sie Aufsätze schreiben. Das heißt sehr verbose (genaue Beschreibung der Variablen: length vs. lengthOfArray, etc...). Die lesen einen Algorithmus wie einen Aufsatz, was mich wiederum stört, denn das hindert mich(!) beim Interpretieren des Algorithmus.