Was ist für euch guter Programmcode?

[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

[Gregor]

simon.phoenix schrieb:

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

Kannst ja irgendwelche Wrapper oder structs oder so nutzen.

[Walli]

simon.phoenix schrieb:

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

Ich meinte das eher auf die Anzahl der Parameter als auf die tatsächliche Länge (in Zeichen) bezogen.
Wenn man Parameterlisten à la WinAPI hat, dann sollte man sich IMHO Gedanken ums Design machen.
Ich habe für mich als grobe Faustregel gesetzt jede Funktion mit mehr als 4 Parametern zu überdenken. Wenn es sich wirklich nicht sinnvoll ändern lässt, dann sind auch mehr Parameter in Ordnung, bei mir aber eher die Ausnahme.

[rüdiger]

@simon
dafür kann man Zeilen umbrechen

struct foo {
  foo(std::string const &host, std::string const &user,
      std::string const &pwd, std::string const &method);
};

find ich deutlich lesbarer, als eine Endloszeile.

[simon.phoenix]

kingruedi schrieb:

@simon
dafür kann man Zeilen umbrechen

struct foo {
  foo(std::string const &host, std::string const &user,
      std::string const &pwd, std::string const &method);
};

find ich deutlich lesbarer, als eine Endloszeile.

Jo klar.

btw: Das nachgestellte const erschwert die Lesbarkeit (Gewöhnungsfaktor, dass man das const vorne an stellt). Finde ich

[Gast]

Bei BigInteger könnte man aber teile in nested classes auslagern. Dadurch wird natürlich nicht der Source der Klasse weniger, aber die Klasse selber trotzdem, was ja entscheidend ist -> man muss über weniger den Überblick behalten. Es geht immer was und sehr oft ist es sinnvoll, etwas möglichst klein zu halten.

Bezüglich der set-Geschichte dachte ich zwischendurch, dass du vielleicht den Rückgabetyp einiger Methoden in meiner Klasse meinst

Nein, das war irgendein krasser Gehirnfehler von mir. Vergiss das einfach *mich nicht darüber zu sprechen trau*

Was hälst du davon? Als Nachteil sehe ich eigentlich nur, dass Sets möglicherweise etwas rechenaufwändiger als Listen sind.

private List<ElementType> getValuesFromNodes(final List<Node> nodeList)
   {
      assert(nodeList != null);
      final LinkedList<ElementType> valueList = new LinkedList<ElementType>();
      for (final Node node : nodeList)
      {
         valueList.add(node.getValue());
      }
      return valueList;
   }

Hier zum Beispiel? Mit was für einem KeyType würdest du das Set dann austatten? Wenn ich mir die public Methoden von Eclipse anzeigen lassen, wird da immer nur Instanzen des ElementType gearbeitet. Du würdest auf jeden Fall irgendwas neues offenbaren.

Naja rechenaufwändiger als an ne doubly LinkedList 50 Dinger einzeln anzuhängen, ich weiß nicht.

[rüdiger]

simon.phoenix schrieb:

btw: Das nachgestellte const erschwert die Lesbarkeit (Gewöhnungsfaktor, dass man das const vorne an stellt). Finde ich

Nein, ein nachgestelltes const ist eigentlich logischer. Vorallem wenn du dir mal anguckst, wie man Member-Funktionen konstant macht.