Was ist für euch guter Programmcode?

[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.

[Gast]

Darfst du jetzt Elemente doppelt haben? Falls nein, warum benutzt du dann kein Set? Ich finde die manuelle Prüfung nicht so toll. Oder verstehe ich was komplett falsch?

assert(type != null);

Alter C++-Hacker

[HumeSikkins]

Shade Of Mine schrieb:

An Humes Code trau ich mich nicht ran

Warun nicht? Zu wild, zu lang oder zuwenig "Fleisch"?

Gerard schrieb:

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.

Jo. Das sehe ich ein. Gute Namen sind das A und O für verständliche Programme. Leider habe ich damit immer wieder große schwierigkeiten. Da es sich hier um eine low-level-Klasse handelt wollte ich neben der Tatsache, dass es sich um eine Int-Menge handelt unbedingt auch etwas über die Implementation in den Namen einbringen. bit_vec_int_set ist dann aber wohl doch etwas gesprächig.

ich habe dafür immer namespaces missbraucht

Das würde sich imo vorallem dann anbieten, wenn man mehrere verschiedene Implementationen hat. So nach dem Motto:

namespace Set {
    class BitVec {...};
    class BinTree {...};
    class SortedList {...};
    ...
}

Imo hat meine Klasse aber mindestens an einer weiteren Stelle ein Namensproblem. SHIFT und MASK gefallen mir eigentlich ganz und garnicht.

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

Wrapper wären sicher nicht schlecht. Allerdings meiner Meinung nach unter einer anderen Motivation. memset und memcpy sagen in diesem Zusammenhang wenig über Semantik. Hier könnten Funktionen wie initNull und copySet vielleicht weiterhelfen.
Dadurch könnte ich die tatsächliche Verwendung von memset und memcpy auch auf einen einzigen Ort beschränken, was wiederum der Korrektheit zu gute kommen würde, da eine Funktion leichter zu testen ist als 23 memset-Aufrufe an 26 verschiedenen Stellen.

[Gregor]

Optimizer schrieb:

Darfst du jetzt Elemente doppelt haben? Falls nein, warum benutzt du dann kein Set? Ich finde die manuelle Prüfung nicht so toll. Oder verstehe ich was komplett falsch?

Was würdest du durch das Set ersetzen? Ich glaube, ich hatte diese Anmerkung von dir bisher falsch interpretiert, deshalb frage ich lieber nochmal nach.

Optimizer schrieb:

Alter C++-Hacker

Öhm... glaube nicht.

[Gast]

Gregor schrieb:

[java]public class DirectedGraph<ElementType> extends AbstractCollection<ElementType>
{
private static enum NeighborType{PREDECESSOR, SUCCESSOR};

private final HashMap<ElementType,Node> nodeMap;

[...]

/**
* This method adds a value to the DirectedGraph.
* @param value This is the value being added to this DirectedGraph. If the value is
* null or the value already exists in this DirectedGraph, the DirectedGraph
* will not be modified.
* @return This method returns true if the DirectedGraph was modified and false if it was not
* modified by this method.
*/
@Override
public boolean add (final ElementType value)
{
if (value == null) return false;
if (nodeMap.keySet().contains(value)) return false;
nodeMap.put (value, new Node (value));
return true;
}

[...]

}
[/java]

Das mit dem Set von mir war Unsinn, vergiss das.

Das ändert aber nichts an dem, was mich hauptsächlich stört. Du prüfst, ob der Key schon vorhanden ist und die Map selber prüft es auch noch mal. Damit ist es IMHO hässlich, unnötig und inperformant, diesen Test durchzuführen.

Java API Dokumentation schrieb:

public V put(K key,
             V value)

    Associates the specified value with the specified key in this map. If the map previously contained
a mapping for this key, the old value is replaced.

    Specified by:
        put in interface Map<K,V>
    Overrides:
        put in class AbstractMap<K,V>

    Parameters:
        key - key with which the specified value is to be associated.
        value - value to be associated with the specified key. 
    Returns:
        previous value associated with specified key, or null if there was no mapping for key. A null
return can also indicate that the HashMap previously associated null with the specified key.

[Gregor]

Optimizer schrieb:

Das ändert aber nichts an dem, was mich hauptsächlich stört. Du prüfst, ob der Key schon vorhanden ist und die Map selber prüft es auch noch mal. Damit ist es IMHO hässlich, unnötig und inperformant, diesen Test durchzuführen.

Ok, was wäre, wenn ich den Test weglassen würde? Ich hätte einen Knoten in meinem Graphen, der vielleicht mit 5 anderen Knoten verbunden ist und jetzt soll der Knoten nochmal neu hinzugefügt werden. Da kommt die Zeile:

nodeMap.put (value, new Node (value));

So. Das hat alle Verbindungen zu den anderen Knoten gelöscht, weil der Knoten hier im Prinzip neu erzeugt wird. ...eigentlich ist es noch viel schlimmer: Man erhält eine Art Phantomknoten, der nicht in der HashMap der Knoten aufgeführt ist, aber immer noch mit dem Graph verbunden ist und den gleichen Wert wie ein anderer Knoten besitzt.

Mit anderen Worten: Ich verstehe nicht, wie man auf den Check so einfach verzichten kann. Kann aber sein, dass ich momentan blind bin. Zeig mal, wie du das meinst.

[Gast]

Stimmt, ich habe da einiges nicht bedacht. Warum benutzt du eigentlich nicht containsKey(Object key)? *kein gutes Haar lass *

[simon.phoenix]

Ach was mir zu Programmierstil noch einfällt:

haltet ihr euch an die "78-zeichen pro zeile"?

[rüdiger]

simon.phoenix schrieb:

haltet ihr euch an die "78-zeichen pro zeile"?

Ja, hat aber IMHO nichts mit dem Programmierstil zu tun.

[Gregor]

Optimizer schrieb:

Warum benutzt du eigentlich nicht containsKey(Object key)?

Da ist was dran. Hab es gleich mal geändert.

Bezüglich der set-Geschichte dachte ich zwischendurch, dass du vielleicht den Rückgabetyp einiger Methoden in meiner Klasse meinst, die momentan eine List zurückliefern. Das wäre nicht so eine einfache Änderung, aber vielleicht sinnvoll. Was hälst du davon? Als Nachteil sehe ich eigentlich nur, dass Sets möglicherweise etwas rechenaufwändiger als Listen sind.

[SirLant]

Ich halte mich nicht an die 80Zeichen ich nehme 90-95, das ist mit den meisten Auflösungen ohne Probleme anzuschauen und wenn wirklich jemand ne kleinere hat (wobei ich das bei nem Programmierer mal einfach bezweifel :p), dann muss er halt gelegentlich etwas scrollen

Wenn man natürlich in Umgebungen arbeitet wo der Code hauptsächlich über terminals bearbeitet wird sollte man es natürlich einhalten, aber warum sollte ich bei nem Programm mit GUI mich so auf diese 80Zeichen beschränken?

@Mecnel (hoffe ich hab den Namen richtig geschrieben):
Es gibt sicher auch mal größere Funktionen, z.B. ein switch-Statement wächst doch ziemlich schnell über die 10Zeilen hinaus und aus dem Grund macht es Sinn das switch-Statement in eine eigene Funktion auszulagern um den eigentlichen Code übersichtlich zu halten

Klassen sollen für gewöhlich auch eher in die breite und nicht in die Länge wachsen, aber dennoch gibt es einfach Klassen die einfach groß sind ohne, dass es sich ändern lässt. Zum Beispiel die BigInteger aus der Java API (~1000Zeilen groß).

Ich weiß ja jetzt nicht wie die Funktion CreateWindowEx intern aussieht aber ich kann mir vorstellen, dass die wirklich so groß sein muss, was aber eher am allgemeinen Design der Winapi liegt *g*

[Walli]

SirLant schrieb:

Klassen sollen für gewöhlich auch eher in die breite und nicht in die Länge wachsen[...]

Meinst du in Form von langen Parameterlisten? Halte ich für noch hässlicher als Klassen mit vielen Methoden.

[Gregor]

MaSTaH schrieb:

SirLant schrieb:

Klassen sollen für gewöhlich auch eher in die breite und nicht in die Länge wachsen[...]

Meinst du in Form von langen Parameterlisten? Halte ich für noch hässlicher als Klassen mit vielen Methoden.

Neee... er meint bestimmt Zeilen, die 500 Zeichen lang sind.

[Walli]

simon.phoenix schrieb:

haltet ihr euch an die "78-zeichen pro zeile"?

Nope, natürlich wachsen meine Zeilen nicht bis ins Unendliche, aber ich mache den Umbruch mehr nach Gefühl. 78 Zeichen halte ich für übertrieben kurz.

[simon.phoenix]

MaSTaH schrieb:

SirLant schrieb:

Klassen sollen für gewöhlich auch eher in die breite und nicht in die Länge wachsen[...]

Meinst du in Form von langen Parameterlisten? Halte ich für noch hässlicher als Klassen mit vielen Methoden.

Längere Parameterlisten lassen sich aber manchmal nicht vermeiden.

z.b. Logon(server, user, pw, method)

dann noch hübsche "const std::string& bla" und schon muss man in die nächste Zeile um die 80 zeichen einzuhalten