Programmierstil

[codechaos]

Die Sache mit dem langen Code habe ich auch auch eher als neutralen Punkt gesehen, hielt es der Vollständigkeit halber dennoch für erwähnenswert.
Meine Meinung zum langen Callstack ist ebenfalls, dass dies kein starkes (wenn überhaupt gültiges) Argument ist, warum das so ist, wurde bereits genannt.

Zu der Sache mit der Verarbeitung der Rückgabewerte kam mir als erstes so etwas in den Sinn:

// ...
Object a, b, c;
List<Object> objectList = new ArrayList<Object>();
fillList(objectList);
for(Object o : objectList) {
	if(isValidA(o)) {
		a = o;
	}else if(isValidB(o)) {
		b = o;
	}else if(isValidC(o)) {
		c = o;
	}
}
// mit a, b, c weiterarbeiten

Es sollen hier aus der Liste ein paar Elemente raus gesucht werden, die bestimmte Eigenschaften erfüllen und anschließend werden die weiterverwendet. Ob man dies mit anderen Mitteln einfacher lösen kann, lassen wir bitte einfach ausser Acht, da es hier um einen anderen Punkt geht

Um die Funktionalität aus dieser Methode auszulagern, sehe ich erst mal zwei Möglichkeiten:

1. drei Methoden, die mir jeweils das Element für a, b oder c liefern
2. Eine Methode, die mir ein Array von Elementen zurück gibt

Bei Variante 1 wird die Liste drei mal durchlaufen, was sich laufzeittechnisch schon nicht richtig anhört.
Variante 2 müsste folgendermaßen weiterverarbeitet werden:

// ...
Object a, b, c;
List<Object> objectList = new ArrayList<Object>();
fillList(objectList);

final Object[] filteredElements = filterList(objectList);
a = filteredElements[0];
b = filteredElements[1];
c = filteredElements[2];
//mit a, b, c weiterarbeiten

Wäre dies die optimale Lösung? Meine Praxiserfahrung ist zwar noch sehr begrenzt und daher weiß ich nicht, wie ich es beschreiben soll, aber auch diese Lösung "fühlt" sich nicht wirklich "richtig" an (bitte verzeiht die Ausdrucksweise, ich weiß ich nicht, wie ich es besser beschreiben soll). In einem Array werden für mich recht "ähnliche" Dinge gespeichert, aber hier werden gefilterte Elemente mit ganz speziellen Eigenschaften zusammen gefasst.

 

[Andi_CH]

Es sind noch viel mehr (~200) - das ist ein Datenhaltungsmoloch der zu 90% aus Variablendeklarationen, gettern und settern besteht und dessen Objekte im ganzen System herumgereicht werden. Keine Struktur nichts.

Einige wenige getter machen einfache Berechnungen oder Zusammenstellungen von Daten, einige setter haben mehr als einen Parameter.
Kommentaranteil (was welche Variable im Gesamtsystem für eine Bedeutung hat) ist <5%

Also - das Problem liegt an gaaaaanz andere Stellen als an einzeiligen Prozedürchen.
Nein, das Ding ist nicht von mir. Und Nein, ich finde es nicht gut

Das ist ein ähnliches Niveau wie ein Statement dass ich irgendwo gefunden habe

(Na ja, ein klein wenig komplizierter war schon codiert, aber das ändert nichts an der Sache.)

if (wert < minimum)
   return false;
else
   return true;

Leute, es hebt mein Ego ungemein wenn ich solchte Statements finde - ich fühl mich dann richtig gut

 

[Andi_CH]

Aber wenn man Beratungsresistenz (nicht dich bezogen) ist und keine andere Meinung gelten darf, kann man auch nichts machen =).

Mich hat geprägt, dass ich seit über 15 Jahren immer nach projektspezifischen und sprachunabhängigen Guidelines gearbeitet habe / arbeiten musste / na ja manchmal auch durfte.

Bis ich mich nur schon daran gewöhnt haben, dass { auf derselben Zeile wie z.B. is stehen darf und } nicht genau darunter sein MUSS

Aber wer sich nur noch um die Detailformattierung des Codes kümmern muss, hat ein recht entspanntest Leben ;-)

Wer keine Argumente anführt welche den Sinn seiner Aussagen untermauern vertritt IMO einen Stand- oder Gesichtspunkt

 

[Gast2]

Bis ich mich nur schon daran gewöhnt haben, dass { auf derselben Zeile wie z.B. is stehen darf und } nicht genau darunter sein MUSS

Stimmt =)... Aber für solche Sachen gibt es auch Codeformatter z.B. für Eclipse, dann haben alle Entwickler die gleiche Formatierung.

 

[bygones]

// ...
Object a, b, c;
List<Object> objectList = new ArrayList<Object>();
fillList(objectList);
for(Object o : objectList) {
    if(isValidA(o)) {
        a = o;
    }else if(isValidB(o)) {
        b = o;
    }else if(isValidC(o)) {
        c = o;
    }
}

ein etwas unglueckliches bsp. So erstmal wirkt es nach design fehler an sich. Umstrukturierung und aenderung ist meist nur mit etwas Kontextwissen moeglich. Die Frage hier ist was passieriert mit a,b,c - ist es sinnvoll eine Zuweisung mehrfach zu ueberschreiben etc. Ich tu mir schwer bei einem solch fiktiven Bsp sinnvolle Antworten zu finden...

 

[Firephoenix]

genauso geht es mir auch bei dem decode-Beispiel von dem anonymen user
Persönlich würde ich sagen "super Gegenbeispiel für schönen Code in einem Framework"

Ohne Kontextwissen ist das Ding mit Sicherheit nicht wartbar, vor allen dingen nicht für jemanden der sich einarbeiten muss.

Und gerade das ist ja auch eins der Hauptthemen von Clean Code (und nein alles davon sehe ich auch nicht in Stein gemeißelt - trotzdem keine schlechte Lektüre) - man schreibt den Code auch für andere - und sollte ihn daher so Strukturieren, dass jemand anderes vergleichsweise schnell die Struktur des Codes erfassen kann, schnell sieht was welche Variable wo tut etc.
Vielleicht kann man das Ding nicht besser schreiben, oder es gibt hier bestimmte Gründe warum die Methode in der Form so da auftaucht - aber wie bygones schon zu dem Code gesagt hat:

Wuerde ich so bei einem internen Codereview nicht durchgehen lassen.

Das sehe ich genauso, wenigstens soviel erkennt man nämlich:
Die Methode macht wesentlich mehr als nur eine kleine Aufgabe, entsprechende Teile lassen sich sicher benennen und auslagern (*warte auf "wäää mein Stack ist so lang"-Geflame*).
Das soll jetzt aber nicht in ein Refactoring von der Methode ausarten, ich finde sie ist nur ein recht passendes Beispiel gegen fast jeden Grundsatz der in Clean Code bzw generell von einigen Leuten hier vertreten wird
Gruß

 

[darekkay]

obwohl das Ctrl-A , Ctrl-I (automatisches Einrücken) längst zum Reflex geworden ist und das Finden von solchen Fehlern massiv vereinfacht.

Ich empfehle dir eine automatische Formatierung beim Speichern einer Datei einzustellen - dadurch sparst du dir diesen Griff, denn ich früher auch sehr oft benutzt habe

 

[codechaos]

// ...
Object a, b, c;
List<Object> objectList = new ArrayList<Object>();
fillList(objectList);
for(Object o : objectList) {
    if(isValidA(o)) {
        a = o;
    }else if(isValidB(o)) {
        b = o;
    }else if(isValidC(o)) {
        c = o;
    }
}

ein etwas unglueckliches bsp. So erstmal wirkt es nach design fehler an sich. Umstrukturierung und aenderung ist meist nur mit etwas Kontextwissen moeglich. Die Frage hier ist was passieriert mit a,b,c - ist es sinnvoll eine Zuweisung mehrfach zu ueberschreiben etc. Ich tu mir schwer bei einem solch fiktiven Bsp sinnvolle Antworten zu finden...

Es ist wirklich ein eher unglückliches Beispiel, gerade weil man nicht weiß, was nachher passieren soll. Mittlerweile denke ich eher, dass es bei so einem Fall eventuell sinnvoller wäre, nicht nur das Filtern, sondern auch die entsprechenden weiteren Arbeitsschritte auszulagern. Sodass es eher eine filterAndDoSomething-Methode ist, die dort aufgerufen wird.

Wie es scheint, ist die Kontra-kurze-methoden-Partei nicht mehr vertreten bzw. übergewandert

 

[knoppers]

Ich empfehle dir eine automatische Formatierung beim Speichern einer Datei einzustellen - dadurch sparst du dir diesen Griff, denn ich früher auch sehr oft benutzt habe

Dies habe ich auch ganz gerne gemacht, bzw. mache ich heute auch noch. Problem ist hier wenn man schon ältere bestehende Projekte bearbeitet und diese dann über Subversion Merged. Hier hat Subversion dann manchmal ein Problem da bei einem Speichervorgang eine ganze Quelldatei formatiert wird. Die Quelldatei vielleicht +- 1000 Code Zeilen hat und vorher noch nie formatiert wurde. Desweiteren hat man hier ein Problem bei prüfen der Historie, da man nicht sofort erkennt wo genau die Änderung geschah.
Nicht desto trotz ist dies eine sehr gute Möglichkeit.

 

[Spacerat]

Lange Methoden mal hin oder her... (Dieser Disput ist wahrhaft lächerlich) mal BTT...
Ging es dem TS nicht irgendwann mal darum, ob diese Klammern besserer Codestyle ist oder nicht. Ich sage mal: "Ja, auf jeden Fall!" und zwar aus folgendem Grund: Dangling (hängendes) else. Ich selbst kenne es nur noch vom "Hören - Sagen" und müsste selbst auch danach Googeln, da ich halt diese Klammern verwende.

if(anything)
  doSomething();
  if(anythingElse)
    doSomethingElse();
else
  doOtherStupidThings();

Kann hier noch irgendjemand genau sagen, zu welchem [c]if[/c] das [c]else[/c] gehört? Stellt euch mal vor, das 2. [c]if[/c] wurde nachträglich eingefügt. OMG
@TS: Tu dir einen grossen Gefallen und verwende diese Klammern... immer! Denn man vergisst auch trotz automatischer Einrückung gerne mal zu welchem [c]if[/c] das [c]else[/c] gehört bzw. gehören soll, nur stört es einen bei der Verwendung der Klammern nicht mehr.
[EDIT]Ok... bei Wikipedia wird das Problem ganz ohne Alsheimer beschrieben... Dangling Else[/EDIT]

 

[knoppers]

Lange Methoden mal hin oder her... (Dieser Disput ist wahrhaft lächerlich) mal BTT...
Ging es dem TS nicht irgendwann mal darum, ob diese Klammern besserer Codestyle ist oder nicht. Ich sage mal: "Ja, auf jeden Fall!8/b]" und zwar aus folgendem Grund: Dangling (hängendes) else. Ich selbst kenne es nur noch vom "Hören - Sagen" und müsste selbst auch danach Googeln, da ich halt diese Klammern verwende.

if(anything)
  doSomething();
  if(anythingElse)
    doSomethingElse();
else
  doOtherStupidThings();

Kann hier noch irgendjemand genau sagen, zu welchem [c]if[/c] das [c]else[/c] gehört? Stellt euch mal vor, das 2. [c]if[/c] wurde nachträglich eingefügt. OMG
@TS: Tu dir einen grossen Gefallen und verwende diese Klammern... immer! Denn man vergisst auch trotz automatischer Einrückung gerne mal zu welchem [c]if[/c] das [c]else[/c] gehört bzw. gehören soll, nur stört es einen bei der Verwendung der Klammern nicht mehr.

Das else gehört zum zweiten if da man ohne Klammerung maximal ein Statement schreiben kann.
Anders wäre dies hier (ohne doSomething):

if(anything)
  if(anythingElse)
    doSomethingElse();
else
  doOtherStupidThings();

hier würde das else zum zweiten if gehören. Probier es in Eclipse aus und formatiere den Code.

 

[darekkay]

Kann hier noch irgendjemand genau sagen, zu welchem [c]if[/c] das [c]else[/c] gehört?

Natürlich, da eine bereits besprochene automatische Einrückung sowas niemals zulassen würde. Das zweite if wäre auf der selben Ebene, wie das erste if und das else. Außedem würde ich Nach doSomething() eine Leerzeile lassen, da die beiden Sachen voneinander unabhängig sind (das anything und anythingElse könnten suggerieren, dass dem nicht so ist (schließlich ist es nicht else if)).

if(anything)
  doSomething();

if(anythingElse)
    doSomethingElse();
else
  doOtherStupidThings();

Ich mag Klammern bei sehr einfachen if-Anweisungen auch nicht. Allerdings ist es ja auch oft so, dass man der if-Anweisung etwas hinzufügen möchte, sei es eine sysout-Anweisung um zu schauen, ob das Programm wirklich die if-Anweisung erreicht. Schreibt man immer die Klammern mit, so reicht ja eine neue Zeile. So muss noch selbstständig { und } eingefügt werden. Aufwand sicherlich nicht sooo hoch, aber auf Dauer könnte es auch stören.

 

[knoppers]

Das else gehört zum zweiten if da man ohne Klammerung maximal ein Statement schreiben kann.
Anders wäre dies hier (ohne doSomething):

if(anything)
  if(anythingElse)
    doSomethingElse();
else
  doOtherStupidThings();

hier würde das else zum zweiten if gehören. Probier es in Eclipse aus und formatiere den Code.

lustig wird es dann wenn es so wird

if (test())
if (testa())
a = "x";
else
a = "y";
else
a = "z";

 

[darekkay]

Bei nur einem else besteht noch die Gefahr, dass man aus Versehen ein Semikolon hinter die erste if-Anweisung macht, dann ist auch alles fürn Popo ^^

So oder so: die Klammern verbessern auf jeden Fall die Lesbarkeit und beugen Tippfehlern vor. Allerdings werde ich einzeilige, unverschachtelte if-Anweisungen immernoch ohne Klammern schreiben, da man bei

if(muh == null)
   return;

...

nun wirklich nichts missverstehen kann.

 

[Gast2]

Bei nur einem else besteht noch die Gefahr, dass man aus Versehen ein Semikolon hinter die erste if-Anweisung macht, dann ist auch alles fürn Popo ^^

So oder so: die Klammern verbessern auf jeden Fall die Lesbarkeit und beugen Tippfehlern vor. Allerdings werde ich einzeilige, unverschachtelte if-Anweisungen immernoch ohne Klammern schreiben, da man bei

if(muh == null)
   return;

...

nun wirklich nichts missverstehen kann.

Wie gesagt wenn man einen Code Formatter(wie die ifs aussehen, wie schleifen,wie lang die zeilen sind usw.) benutzt hat jeder Entwickler die gleiche Formattierung und man kann keine eigenen Sachen machen, eine gescheite IDE nimmt einen auch sowas ab.