Was ist für euch guter Programmcode?

[looooool]

Mecnels schrieb:

Shade Of Mine schrieb:

Woher hast du das?

Nun ja, ich war mal in der Compilerentwicklung tätig. Du auch?

ROFL

[Gast]

Shade Of Mine schrieb:

Optimizer schrieb:

* @param file Der ursprüngliche Dateiname.
[...]
	protected static File findValidFileName(File file)

Wirklich der _Dateiname_?

ok...

/**
	 * Schließt ein Socket und fängt jede IOException, die dabei auftritt.<br>
	 * Leider implementiert <code>ServerSocket</code> nicht das
	 * <code>Closeable</code>-Interface, sonst wäre diese Methode überflüssig.
	 * @param socket Ein Socket, das geschlossen werden soll. Ist dieser
	 * Parameter <code>null</code>, so macht die Methode gar nichts.
	 */

Manchmal rulen templates schon, gell?

Damit hast du nicht wirklich nen Vorteil. Du würdest dir hier 2 Methoden sparen. Aber du kannst den Namen genauso wenig ändern und wenn du es tust und vielleicht was anderes so nennst, oder etwas an der Funktion erweiterst, hast du Verhalten, was du vielleicht nicht wolltest, aber lassen wir das... wir hatten das ja schon mal vor kurzem. IMHO ist es einfach ein Designfehler, dass die Sockets das Interface nicht implementieren.

Warum ist das eine Ausnahme?
validate*
klingt für mich nach: prüfe ob es passt und sags mir dann
hier wäre der name
ensureValidProtocol()
wohl besser, denn ein validate* klingt für mich nicht als ob
es eine Ausnahme wäre, wenn die Protokolle nicht passen...

Da hast vielleicht Recht.

2. kleiner seitenhieb auf checked exceptions.
   dein Kommentar
   // (Andere Exception werfen)
   ist nicht durchführbar, weil du bereits IsNotAFolderException als einzig
   mögliche Exception definiert hast. Eine Änderung hier, würde client code brechen.

Das ist genau das Misverständniss, was mich so ärgert.
Angenommen, die Exceptions wären nicht checked. Cool, dann kann ich jetzt ja einfach noch nen anderen Ausnahmetypen werfen!!
Aber wer fängt sie? Hast du darüber schon mal nachgedacht? Hast du dann nicht mit Client Code gebrochen, wenn der plötzlich abstirbt, weil du einfach mal schnell noch ne neue Exception werfen musstest? Ein sehr ungelungener Seitenhieb, wie ich finde.
Denn mit Client Code brichst du bei sowas immer. Es ist nur ne Frage, ob es bemerkt wird, oder nicht.
btw. ist es hier natürlich völlig egal, weil die Methode eh private ist und ich das selber schon auf die Reihe krieg. Außerdem ist die Methode schlicht noch nicht fertig. Aber ich weiß schon, was du sagen wolltest, nur leuchten mir deine Argumente gegen checked Exceptions nie ein.

Mir persönlich gefallen diese Optional Komponenten Progressbar & Co nicht.
Du bindest dich damit an Swing - dh, eine Terminalausgabe erschwerst du damit
ungeheuerlich.

Ja, da hab ich auch länger rumüberlegt. Ich wollte erst ein Interface für solche Status-Komponenten machen, was die Aktionen dann fressen.
Dann hätte ich noch Klassen geschrieben, die das implementieren und die Swing-Komponenten entsprechend dann ändern. Ich habe zu Gunsten der Einfachheit davon abgesehen, aber ich stimme dir zu.

Danke, Code Review rult.

[Walli]

ness schrieb:

Und dann gibt es ja noch inline...

inline ist ausserdem nur eine Empfehlung und in den meisten Fällen unnötig, da der Compiler meist ganz gut erkennt was er expandieren soll und was nicht.

[Gast]

Mecnels schrieb:

Shade Of Mine schrieb:

Woher hast du das?

Nun ja, ich war mal in der Compilerentwicklung tätig. Du auch?

lölölölölölöl
Welcome back, Sgt. Nukem.

[Mecnels]

Mecnels schrieb:

Wenns ans optimieren geht, sieht das ganze dann schon wieder anders aus. Aber ich denke es heißt nicht ohne Grund, eine verfrühte optimierung ist der Ursprung allesn Übels...
Und dann gibt es ja noch inline...

Du wirst es nicht glauben, es gibt Problemstellungen, in denen kommt perfekte Speicher- und Zeiteinteilung an erster Stelle (verfrühte Optimierung). Auch wenn Dir der zur Verfügung stehende Hauptspeicher grenzenlos erscheinen mag, einige Berechnungen überfordern nach wie vor binnen weniger Millisekunden 2048 MB grosse RAMs (versuche ein Schachprogramm zu schreiben, das in eine feste Zugtiefe von 15 Halbzügen vordringt, und Du weisst, worauf ich hinaus will).

[Walli]

Und du wirst nicht glauben, dass findige Leute nur da optimieren wo es Sinn macht und dass man misst bevor man optimiert. Meist macht der Compiler schöne Optimierungen schon von selber; nett von ihm, oder? Dir als Compilerentwickler sage ich da aber sicher nichts neues. Ausserdem kann ein guter Algorithmus meist viel mehr zur Optimierung beitragen als ein Codeklumpen den kein Schwein mehr lesen kann.

[Gast]

Jetzt hast du schon zweimal korrigiert und es fehlt immer noch mindestens ein Komma.

[Walli]

Optimizer schrieb:

Jetzt hast du schon zweimal korrigiert und es fehlt immer noch mindestens ein Komma.

Neue Rechtschreibung *bluff*

[simon.phoenix]

@mecnels

Deine Signatur ist etwas großkotzig, sorry...

[Der es besser weiss]

Es gibt keinen guten oder schlechten Code.
Es gibt nur Code, der eine Problemstellung zufriedenstellend löst,
oder eben nicht zufriedenstellend löst. Ob da jetzt viele Schleifen oder
viele Funktionen oder viel Flammerei mit im Spiel ist, ist dann eigentlich
belanglos!!!

Schluss jetzt!!!

[Walli]

simon.phoenix schrieb:

@mecnels

Deine Signatur ist etwas großkotzig, sorry...

~vorsicht ironie~
Wieso? Ich hätte gerne ein top-optimiertes Stratego von dessen Source ich noch was in puncto Optimierung lernen kann .

[Mecnels]

simon.phoenix schrieb:

@mecnels

Deine Signatur ist etwas großkotzig, sorry...

Dass mittlerweile Aufrichtigkeit mit Grosskotzigkeit gleichgesetzt wird, ist sogar mir neu, und ich versuche doch immer einigermaßen auf dem Laufenden zu bleiben.

Jedenfalls ist es nicht irgendeine Phantasie meinerseits, dass Speicherzugriffe auf ein Prozessorregister beinahe mit der einem arglosen Kunden verkauften Taktfrequenz (sagen wir 2.5 Ghz) passieren (im Unterschied zu Zugriffen auf den Haupt- oder Grafikkartenspeicher - na, was glaubt ihr: wieviele 16-Bit-Werte kann ein Prozessor, auf dem 2.5 Ghz draufstehen, pro Sekunde aus dem Hauptspeicher in ein ein Prozessorregister holen?)

Preisfrage 1: Was ist eine Game Loop?
Preisfrage 2: Was macht die Funktion QueryPerformanceCounter? (ein sperriger
Bezeichner, aber sehr aussagekräftig, wie ich meine)
Das beste zum Schluss:
Preisfrage 3: Was glaubt ihr, wieviele Codezeilen in C stecken in der wohl-
bekannten und vielgenutzten CreateWindowEx- Funktion?
(Etwa auch nur 10? Weil ja alles das selbe ist? Oder nicht
doch eher 2741 Codezeilen_wenn ich mich nicht verzählt hab?
Tja - wer weiß? Nicht mitgerechnet
sind alle Funktionen, die CreateWindowEx selber aufruft!!!)

Seltsam: Wenn sich wirklich jedes Problem in 10 Zeilen C++ Code lösen lassen könnte (weil ja TicTacToe, Stratego und womöglich sogar Schach ein und dasselbe sind), wäre wohl nie jemals ein Window auf einem Screen aufgetaucht, weil es keine Funktion gegeben hätte, die es erzeugt!

Flammen bringt aber wirklich niemanden hier weiter, also lass ich Euch den Spass und ihr dürft Euch ohne meine weitere Einwirkung weiter gegenseitig die Flöhe in die Ohren setzen.

Trotzdem einen schönen Abend.

[Gast]

Seltsam: Wenn sich wirklich jedes Problem in 10 Zeilen C++ Code lösen lassen könnte (weil ja TicTacToe, Stratego und womöglich sogar Schach ein und dasselbe sind), wäre wohl nie jemals ein Window auf einem Screen aufgetaucht, weil es keine Funktion gegeben hätte, die es erzeugt!

Musst du denn alles wörtlich nehmen? Es ist ja nicht so, dass eine Funktion nicht länger als 10 Zeilen sein darf. Natürlich ist es möglich, jede Funktion unter 10 Zeilen zu haalten aber wohl in dem Maße nicht sinnvoll. Aber wenn sie über 100 ist, dann läuft einfach was falsch, dann sollte man Teile in andere Funktionen auslagern, um
- mehr Übersichtlichkeit zu haben
- solche gelösten Teilaufgaben woanders wiederverwenden kann
- nicht komplett verrückt wird

Das ein Programm dadurch langsamer wird, ist natürlich völliger Blödsinn. Compilerbauer bist du mit Sicherheit nicht, denn fast jeder weiß es besser als du.

Preisfrage 1: Was ist eine Game Loop?

Willst du mich vergeigen?

Preisfrage 2: Was macht die Funktion QueryPerformanceCounter? (ein sperriger
Bezeichner, aber sehr aussagekräftig, wie ich meine)

Ich weiß es. Ist das jetzt irgendwie toll?

Preisfrage 3: Was glaubt ihr, wieviele Codezeilen in C stecken in der wohl-
bekannten und vielgenutzten CreateWindowEx- Funktion?

Wenn du Microsoft-Code als Vorbild nimmst, bist du verloren.

P.S. Nicht persönlich gemeint, aber du bist wirklich irgendwie seltsam. Du "lässt uns unseren Spaß" und merkst gar nicht, dass das hier fast alles gegen dich gerichtet ist - weil du Stuss erzählst.

Dass mittlerweile Aufrichtigkeit mit Grosskotzigkeit gleichgesetzt wird, ist sogar mir neu

Was hat deine Signatur mit aufrichtig zu tun? Gut, man muss es ja nicht gleich als großkotzig beschreiben, meine Reaktion war eher sowas wie "na und?", also nicht dramatisch.

Naja, ebenfalls nen schönen Abend.

@Mods: Bitte den Thread teilen! Ich möchte noch über code-style diskutieren.

[borg]

imo geht ihr da alle zu ideologisch ran.

und Mecnels:
es sprechen alle von birnen und du stellst auf einmal fragen zu pflaumen. da soll einer schlau draus werden.

[Walli]

Mecnels schrieb:

Preisfrage 3: Was glaubt ihr, wieviele Codezeilen in C stecken in der wohl-
bekannten und vielgenutzten CreateWindowEx- Funktion?
(Etwa auch nur 10? Weil ja alles das selbe ist? Oder nicht
doch eher 2741 Codezeilen_wenn ich mich nicht verzählt hab?
Tja - wer weiß? Nicht mitgerechnet
sind alle Funktionen, die CreateWindowEx selber aufruft!!!)

Prust, jetzt ist auch meine letzte Befürchtung bestätigt. Du denkst nicht selber nach warum Code-Strukturierung sinnvoll ist sondern verweist auf die ach so tolle WinAPI .

Zu 1 und 2: Ich weiß was beides ist. Bekomme ich jetzt nen Keks?

[Mecnels]

Optimizer schrieb:

P.S. Nicht persönlich gemeint, aber du bist wirklich irgendwie seltsam. Du "lässt uns unseren Spaß" und merkst gar nicht, dass das hier fast alles gegen dich gerichtet ist - weil du Stuss erzählst.

Das was eine sinnvolle Diskussion über ein Thema braucht, sind Beispiele. Ich hab damit angefangen, ja, ich war dumm genug, einen Auszug aus meinem Code zu posten. Auch auf die 'Gefahr' hin, dass sich damit viele gar nicht anfreunden können, und den Code als vorbildhaft schlecht beurteilen werden. Einige andere haben dann auch angefangen, Auszüge aus ihren Programmen zu posten, ebenfalls mit der Intention, ein Beispiel abzuliefern, über das überhaupt diskutiert werden kann. Denen ging es denn auch nicht viel besser (zur heiligen Mantra _ was macht diese if denn hier - so ähnlich sahen die Kommentare dazu aus). Wenn Du ehrlich bist, würden auch an kurzen Auszügen aus Deinen Quellcodes kein gutes Haar gelassen werden, denn irgend etwas passt natürlich irgend jemandem der es ja so viel besser weiss überhaupt nicht daran.
Ich glaube, die meisten hier sind zu feige, um eine Kostprobe ihres Programmierkönnens (das zweifellos bei allen auf einem hohen Level anzusiedeln ist) in Form eines kurzen Quelltexts zu posten, über den dann überhaupt erst eine Diskussion geführt werden könnte (es hat irgendwie etwas Entlarvendes an sich, nicht? ich könnte vielleicht doch nicht so toll programmieren wie ich glaube und dann ziehen die über mich her? da halte ich mich doch lieber im Hintergrund und kritisiere lieber den Quellcode derjenigen, die dumm genug sind, einen konstruktiven Beitrag zur Diskussion liefern zu wollen! - Ich kann mich natürlich auch darauf beschränken nur die Beiträge anderer zu kritisieren und herab zu machen, dabei mit Scheinwissen um mich zu werfen, mir dabei gut vorkommen, und damit nicht einmal in die Kritik zu geraten, weil ich ja nur einer von vielen bin die das machen und deshalb nicht auffalle!).
Nur wo ein konkreter Gegenstand besteht, über den diskutiert werden kann, macht auch eine Diskussion Sinn und führt zu Ergebnissen, die für die alle Diskussionsteilnehmer lehrreich sein können.

Dass mittlerweile Aufrichtigkeit mit Grosskotzigkeit gleichgesetzt wird, ist sogar mir neu - Was hat deine Signatur mit aufrichtig zu tun? Gut, man muss es ja nicht gleich als großkotzig beschreiben, meine Reaktion war eher sowas wie "na und?", also nicht dramatisch.

Naja, ebenfalls nen schönen Abend.

a) Ich bin wirklich 59.
b) Gute Nacht.

[Shade Of Mine]

Mecnels schrieb:

Das was eine sinnvolle Diskussion über ein Thema braucht, sind Beispiele.

Sind ja einige hier, wo ist das Problem.

Dass Optimizers Code gut ist, habe zumindest ich festgestellt.
Natürlich gibt es immer Kritikpunkte, wie zB an simon.phoenixs Code gab es halt einen Designschnitzer (Die Exceptionhandhabung) Macht aber nix.

Ich hab damit angefangen, ja, ich war dumm genug, einen Auszug aus meinem Code zu posten.

An Humes Code trau ich mich nicht ran, aber die anderen Codes habe ich so fair es mir möglich war betrachtet. Und wenn du dich hier umsiehst, so werden zumindest die meisten meine Auffassung im großen und ganzen teilen (natürlich werden kaum 2 leute die exakt selbe Meinung haben).

Denen ging es denn auch nicht viel besser (zur heiligen Mantra _ was macht diese if denn hier - so ähnlich sahen die Kommentare dazu aus).

Naja, wenn die einzige Kritik ist:
"das if würde ich weglassen"
dann ist der code wohl ziemlich ideal.

Ich glaube, die meisten hier sind zu feige, um eine Kostprobe ihres Programmierkönnens (das zweifellos bei allen auf einem hohen Level anzusiedeln ist) in Form eines kurzen Quelltexts zu posten, über den dann überhaupt erst eine Diskussion geführt werden könnte (es hat irgendwie etwas Entlarvendes an sich, nicht?

Glaube ich nicht. Viele haben uns schonmal Code zum fraß vorgeworfen und viele werden es noch tun.

Es hat ja auch damit zutun, ob man gerade ordentlichen Code da hat.
Ich habe zB neben dem alten TicTacToe nur noch einen anderen C++ code da. Meine Auswahl etwas zu posten ist also beschränkt. andere haben garkeinen aktuellen code bei der hand (und zu alter code bringt nichts, weil sich der style inzwischen geändert haben kann).

Ich kann mich natürlich auch darauf beschränken nur die Beiträge anderer zu kritisieren und herab zu machen, dabei mit Scheinwissen um mich zu werfen, mir dabei gut vorkommen, und damit nicht einmal in die Kritik zu geraten, weil ich ja nur einer von vielen bin die das machen und deshalb nicht auffalle!).

Du würdest indem momeent auffallen, indem nachgefragt wird und du erklären müsstest warum deine lösung besser als die vorhandene ist.

Nur wo ein konkreter Gegenstand besteht, über den diskutiert werden kann, macht auch eine Diskussion Sinn und führt zu Ergebnissen, die für die alle Diskussionsteilnehmer lehrreich sein können.

Tja, guter Code ist eben nicht konkret.
Dennoch gibt es Tendenzen und variablen nicht lokal deklarieren ist sicher ein zeichen für schlechten code.
kein einheitliches namensschema ist sicher auch schlecht
zu tief verschachtelte schleifen sind sicher nicht leicht wartbar
mehr als eine aufgabe pro funktion ist auch sicherlich nicht ideal
kommentare wie
"} // Schliessende Klammer der for-Schleife mit i "
bringen auch nicht wirklich viel
variablennamen für koordinanten sind wohl x und y besser als i und j.
auch ist m nicht sehr aussagekräftig

wir können gerne über deinen code diskutieren, es gibt eine menge zu diskutieren
das ist natürlich nicht böse gemeint. aber wenn jemand hier code postet, dann will er lernen, sonst bringt es ja nichts.
es sei denn der code ist so super, dass alle nur staunen das wage ich aber zu bezweifeln, weil jeder fehler macht.

btw: ich finde jeder soll seine signatur so schreiben können wie er will. und Mecnels' signatur ist doch voll in ordnung. nicht alltäglich und man wird sich denken "na und?" aber sicher nicht provozierend oder großkotzig.

[Gerard]

HumeSikkins schrieb:

Cool,
leider sind alle meine interessanten Sachen die ich gerne mal reviewen lassen würde mit viel Kontext verbunden. Auf der anderen Seite möchte ich diese Gelegenheit für einen kostenlosen Code-Review aber auch nicht ungenutzt verstreichen lassen.

ich versuche es mal

class bit_vec_int_set
{
public:
	// creates a new empty set for integers in the range [0, capacity()]
	// where capacity() is >= maxVal
	// Post: size() == 0 && capacity() >= maxVal
	explicit bit_vec_int_set(UInt32 maxVal = 512);
	bit_vec_int_set(const bit_vec_int_set& other);
	~bit_vec_int_set();
	bit_vec_int_set& operator=(const bit_vec_int_set&);

der name bit_vec_int_set ist gut, wenn jemand code liest der diese klasse benutzt, wird er den code schneller verstehen können, dank des namen.
die implementierung leidet aber etwas unter den namen, vor lauter bit_vec_int_set sieht man den wald nicht.
ich habe dafür immer namespaces missbraucht
in der implementierung konnte ich dann namen benutzen die etwas kürtzer waren. außerhalb der implementierung habe ich den namespace ausgeschrieben

punkt zwei ist das du memset, memcmp usw. benutzt, ein wrapper um diese funktionen mit ein paar asserts würde nicht schaden, die funktionen sind nicht ohne und einige fehler könnten sich erst später bemerkbar machen
wie z.b. man will etwas mit 0 füllen: memset(p, sizeof(*p)*10, 0) oder memset(p, 0, sizeof(p)*10); usw.
ok mir fällt zwar nicht ein wie ich so was mit ein wrapper abfangen kann, aber vielleicht bin ich auch schon zu eingerostet

[Gregor]

Mecnels schrieb:

Mir fällt auf, dass viele von Euch 'schleifenscheu' sind, das heisst: in den geposteten Beispielcodes ist zwar alles ganz nett in kleine Teilfunktionen zergliedert, aber kaum einmal entdecke ich (z.B. beim Programm mit den gerichteten Graphen) geschachtelte Schleifen.

[...]

Unverständlich ist mir deshalb die Meinung, schleifenreicher Code sei schlecht, denn in jedem Fall ist er locker um das Doppelte leistungsstärker als Code, der mit Funktionsbezeichnern (Adressen) nur so überhäuft ist.

Jedenfalls ist auch der Code mit dem gerichteten Graphen sehr ordentlich und für einen Aussenstehenden, der nur Grundkenntnisse in Java hat, leicht nachvollziehbar. Hut ab!

Danke für das Lob, aber...

Die Kritik war leider gerade an der Stelle mit den verschachtelten Schleifen komplett gerechtfertigt. Der Code ist da nicht nur unübersichtlich, sondern sogar falsch, wie ich eben gemerkt habe.

Ich hatte bisher keine Unit-Tests für diese Klasse geschrieben, habe es jetzt aber gemacht, weil ich an dieser Methode sowieso ein kleines Refactoring durchführen wollte. Bisher hatte ich die Klasse immer nur für Graphen ohne Zyklen genutzt, was ich aber in dem entsprechenden Unit-Test nicht gemacht habe. Das Resultat ist, dass diese Methode für Graphen mit Zyklen eine Endlosschleife enthalten kann. Das ist mir etwa ein Jahr lang nicht aufgefallen, da sieht man also mal, wie wichtig Unit-Tests sind.

Das zeigt aber auch, dass die Methode ansich zu kompliziert ist: Bei unkomplizierterem Code werden solche Fehler leichter gefunden (teilweise auch ohne Tests).

*wie peinlich, dass mir das erst jetzt auffällt* ...ich werde die Methode mal komplett neu und anders schreiben.

[Gregor]

So. Hier die neue Variante der entsprechenden Methode bzw. jetzt 2 Methoden:

private List<ElementType> getAllNeighbors (final NeighborType type,
                                              final ElementType value)
   {
      assert(type != null);
      final Node node = nodeMap.get(value);
      if (node == null) return null;
      final HashMap<ElementType,Boolean> neighborMap = 
         new HashMap<ElementType,Boolean>(2*nodeMap.size());
      for(final ElementType key : nodeMap.keySet())
      {
         neighborMap.put(key,false);
      }
      markAllNeighbors(type,node,neighborMap);
      final List<ElementType> neighborList = new LinkedList<ElementType>();
      for(final ElementType currentValue : neighborMap.keySet())
      {
         if(neighborMap.get(currentValue)) neighborList.add(currentValue);
      }
      return neighborList;
   }

   private void markAllNeighbors(final NeighborType type, final Node node,
                                 final HashMap<ElementType,Boolean> neighborMap)
   {
      assert(type != null);
      assert(node != null);
      assert(neighborMap != null);
      final List<Node> currentNeighbors = node.getNeighborNodes(type);
      for(final Node currentNode : currentNeighbors)
      {
         final ElementType currentValue = currentNode.getValue();
         if (!neighborMap.get(currentValue))
         {
            neighborMap.put(currentValue,true);
            markAllNeighbors(type,currentNode,neighborMap);
         }
      }
   }

Sieht IMHO schon etwas besser aus und der Test wird jetzt auch bestanden.