Was ist für euch guter Programmcode?

[ich nicht]

Wer findet den Code von Mecnels gut?

[Walli]

Ich auch nicht. Zu tief verschachtelt und zu wenig in sinnvolle Funktionen ausgelagert.

[trenner]

Was ich total schrecklich finde ist das er das SendMessage und MessageBox da rein programmiert hat.

[nix da]

Hi,

ich finde Einheitlichkeit sehr wichtig wie z.B. hier:

// Testet ob die Zahl gerade oder Ungerade ist.
template<typename T> inline const bool isEven (const T& value)
{ return (!(value&1)); }

    // Eine Absolut Funktion.
template<typename T> inline const T abs (const T& value)
{ return ((value < 0) ? -value : value); }

    // Rechnet value^2.
template<typename T> inline const T square (const T& value)
{ return (value*value); }

[jaaaa]

Toooooollllllll

[nix da]

Du sagst toll, aber willste mal das Gegenstück zur Ordnung sehen?

// Testet ob die Zahl gerade oder Ungerade ist.
template<typename T> inline bool isEven(const T& value) { return !(value&1) ; }

template<typename T> inline T abs (const T value)
{ 
    // Eine Absolut Funktion.
    return value < 0 ? -value : value; 
}

// Rechnet value^2.
template<typename T> T square (T value){ 
    return (value*value); 
}

Ist doch Chaos, und vorallem bei diversen OpenSources sieht man so einen Müll.

Für mich muss guter und ordentlicher Code einheitlich sein, erweiterbar sowie durchdacht und wiederverwertbar (also für die nächste 3-4 Jahre)

[Mecnels]

MaSTaH schrieb:

Ich auch nicht. Zu tief verschachtelt und zu wenig in sinnvolle Funktionen ausgelagert.

Was ist denn eine sinnvolle Funktion? Eine, mit der ich einzelne Zahlen in eine Klasse schreiben und abfragen kann? Oder eine Funktion, die bei ihrem Aufruf vollautomatisch einen KI gesteuerten Spielzug generiert? Eine Funktion des letzteren Typs muss natuerlich ein bisschen laenger sein, glaubst Du nicht? (Natuerlich operiert auch mein Programm auf der untersten Ebene mit so elementaren Setz und Lesefunktionen)

Zu: Total schrecklich finde ich, dass er das MessageBox da hineinprogrammiert hat. Zum Glück des Spielers spielt das Programm stark genug, dass diese MessageBox ohnehin nie erscheint. (Nur ein Scherz, aber kannst es ja trotzdem gerne versuchen http://www.geocities.com/c_pruell/MecnelsTactics.zip).

Vielleicht postet einfach jeder einen typischen, durchschnittlichen, kurzen Auszug aus seinen Programmen, und irgendwann küren wir dann den 'übersichtlichsten Code'. Na?

Netten Abend noch.

[Blue-Tiger]

Mecnels schrieb:

MaSTaH schrieb:

Ich auch nicht. Zu tief verschachtelt und zu wenig in sinnvolle Funktionen ausgelagert.

Was ist denn eine sinnvolle Funktion? Eine, mit der ich einzelne Zahlen in eine Klasse schreiben und abfragen kann? Oder eine Funktion, die bei ihrem Aufruf vollautomatisch einen KI gesteuerten Spielzug generiert? Eine Funktion des letzteren Typs muss natuerlich ein bisschen laenger sein, glaubst Du nicht? (Natuerlich operiert auch mein Programm auf der untersten Ebene mit so elementaren Setz und Lesefunktionen)

Zu: Total schrecklich finde ich, dass er das MessageBox da hineinprogrammiert hat. Zum Glück des Spielers spielt das Programm stark genug, dass diese MessageBox ohnehin nie erscheint. (Nur ein Scherz, aber kannst es ja trotzdem gerne versuchen http://www.geocities.com/c_pruell/MecnelsTactics.zip).

Vielleicht postet einfach jeder einen typischen, durchschnittlichen, kurzen Auszug aus seinen Programmen, und irgendwann küren wir dann den 'übersichtlichsten Code'. Na?

Netten Abend noch.

er wollte dir doch nur Tipps geben. Immerhin gibs noch den Merksatz "One class/function, one responsability"... will heissen: eine Funktion sollte immer nur fuer eine einzige Sache zustaendig sein. Das hat mehrere Vorteile, z. B. kann man aus einem Methodennamen wie "Computer_zieht()" nicht gleich riechen, dass da auch Output gemacht wird, sonst muesste die Funktion ja "Computer_zieht_und_benachrichtigt_Wenn_der_Computer_verliert()" heissen. Weil der Funktionsname ja beschreiben soll, was die Funktion macht...

Wesentlich sinnvoller waer's, wenn du z. B. eine Exception werfen wuerdest (genau fuer sowas sind sie ja da), oder wenigstens einen Rueckgabewert verwendest (true wenn der Computer ziehen konnte, false wenn nicht), und das dann auswertest. Das waer IMO leicht verstaendlich

Das war auch gemeint mit "sinnvolle Funktionen": wenn eine Funktion zu viel macht, dann sollte man sie besser in kleinere Unterfunktionen aufteilen. In deinem konkretem Fall koenntest du z. B. den gesamten Punkt b) in eine andere Funktion auslagern. Kuerzere Funktionen sind viel, viel leichter zu verstehen

[manni4]

Blue-Tiger: In welchem Fall soll er eine Exception werfen?

[Shade Of Mine]

Mecnels schrieb:

Was ist denn eine sinnvolle Funktion? Eine, mit der ich einzelne Zahlen in eine Klasse schreiben und abfragen kann? Oder eine Funktion, die bei ihrem Aufruf vollautomatisch einen KI gesteuerten Spielzug generiert? Eine Funktion des letzteren Typs muss natuerlich ein bisschen laenger sein, glaubst Du nicht? (Natuerlich operiert auch mein Programm auf der untersten Ebene mit so elementaren Setz und Lesefunktionen)

Deine Funktion ist zu komplex und die variablennamen nichtssagen und inkonsequent. deine kommentare erzählen romane und schleifen sind unlesbar.

der trick ist: auch eine KI funktion muss nicht mehr als 10 zeilen haben.
die einzigen langen funktionen bei mir (mit lange meine ich 20 zeilen) sind initialisierungs funktionen, die somit keine logik enthalten.

Vielleicht postet einfach jeder einen typischen, durchschnittlichen, kurzen Auszug aus seinen Programmen, und irgendwann küren wir dann den 'übersichtlichsten Code'. Na?

Kannst dir ja mal das TicTacToe aus meinem Tutorial ansehen, ist zwar nicht eine entgültige version - dank den leuten hier im forum gabs viele verbesserungen (die sind aber nicht online)

so sieht etwa ein programm bei mir aus.
du wirst erkennen, dass ich nahezu keine kommentare brauche und relativ kurze klare funktionen habe.
dazu wird auch noch alles plattformunabhängig gehalten

da kannst du noch eine menge lernen (obwohl es, wie gesagt, noch kein wirklicher vorzeigecode ist)

[nman]

nix da schrieb:

vorallem bei diversen OpenSources sieht man so einen Müll.

Cool, das muss ich mir merken!
Dass man bei ClosedSource definitionsgemäß solchen Müll nicht sieht, ist Dir aber schon klar, hm?

[nix dar]

Natürlich. Wenn der Closed Source offen wäre, würde man es da genauso finden.

[nix da]

nman
Du machst mich noch zum Raucher

[Gregor]

Mecnels schrieb:

Vielleicht postet einfach jeder einen typischen, durchschnittlichen, kurzen Auszug aus seinen Programmen, und irgendwann küren wir dann den 'übersichtlichsten Code'. Na?

Ich fang mal an. Von mir gibt es aber nur Java-Code. Die Klasse ist nicht komplett fertig, aber was da ist sollte funktionieren. Konstruktive Kritik ist selbstverständlich erwünscht.

package container;

import java.util.*;

/**
 * This class describes a directed graph. It is not synchronized.
 * @param <ElementType> This is the type of the values that can be nodes of
 *                      the DirectedGraph.
 * @author Gregor
 * @version 0.1
 */
public class DirectedGraph<ElementType> extends AbstractCollection<ElementType>
{
   private static enum NeighborType{PREDECESSOR, SUCCESSOR};

   private final HashMap<ElementType,Node> nodeMap;

   /**
    * This constructor constructs a new DirectedGraph with the initial capacity of a
    * HashMap constructed by the default constructor.
    */
   public DirectedGraph ()
   {
      nodeMap = new HashMap<ElementType,Node> ();
   }

   /**
    * This constructor constructs a new DirectedGraph with the given initial capacity.
    * @param initialCapacity This is initial capacity of the new DirectedGraph. The
    *                        intitialCapacity may not be negative.
    */
   public DirectedGraph (final int initialCapacity)
   {
      if (initialCapacity < 0) throw new IllegalArgumentException("InitialCapacity is negative.");
      nodeMap = new HashMap<ElementType,Node> (initialCapacity);
   }

   /**
    * 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;
   }

   /**
    * This method removes all values and edges from this DirectedGraph.
    */
   @Override
   public void clear ()
   {
      nodeMap.clear();
   }

   /**
    * This method tests if the given value exists in the DirectedGraph.
    * @param value This is the value to be tested.
    * @return The method returns true if value is found in this DirectedGraph and false if not.
    */
   @Override
   public boolean contains (final Object value)
   {
      return nodeMap.keySet().contains(value);
   }

   /**
    * This method returns the number of values in this DirectedGraph.
    * @return The number of values is returned.
    */
   @Override
   public int size()
   {
      return nodeMap.size();
   }

   /**
    * This method removes the given value from this DirectedGraph if it is conained in
    * the DirectedGraph.
    * @param value This is the value that will be removed from the DirectedGraph by this 
    *              Method.
    * @return The method returns true if it modified the DirectedGraph. This is the case
    *         if the value is not null and the value was contained in the DirectedGraph 
    *         before calling this method. The method returns false if the DirectedGraph 
    *         was not modified by this method.
    */
   @Override
   public boolean remove (final Object value)
   {
      final Node node = nodeMap.get(value);
      if (node == null) return false;
      nodeMap.remove(value);
      removeConnections(node);
      return true;
   }

   private void removeConnections (final Node node)
   {
      final LinkedList<Node> successorList = node.getNeighborNodes(NeighborType.SUCCESSOR);
      final LinkedList<Node> predecessorList = node.getNeighborNodes(NeighborType.PREDECESSOR);
      for (final Node currentNode : successorList)
      {
         currentNode.removeNeighborNode(NeighborType.PREDECESSOR,node);
      }
      for (final Node currentNode : predecessorList)
      {
         currentNode.removeNeighborNode(NeighborType.SUCCESSOR,node);
      }
   }

   /**
    * This method creates an Iterator that iterates over the values contained in this 
    * DirectedGraph.
    * @return The method returns the iterator for this DirectedGraph.
    */
   @Override
   public Iterator<ElementType> iterator()
   {
      return new GraphIterator ();
   }

   /**
    * This method adds a new edge between the given predecessor and successor to this
    * DirectedGraph. If predecessor or successor are not yet part of this DirectedGraph
    * they will be added.
    * @param predecessor This is the value that is the starting-point of the new edge. 
    *                    The predecessor may not be null.
    * @param successor This is the value that is the end-point of the new edge. The 
    *                  successor may not be null.
    */
   public void addEdge (final ElementType predecessor, final ElementType successor)
   {
      if (predecessor == null) throw new NullPointerException("Predecessor is null.");
      if (successor == null) throw new NullPointerException("Successor is null.");
      add(predecessor);
      add(successor);
      final Node predecessorNode = nodeMap.get(predecessor);
      final Node successorNode = nodeMap.get(successor);
      assert(predecessorNode != null);
      assert(successorNode != null);
      predecessorNode.addNeighborNode(NeighborType.SUCCESSOR,successorNode);
      successorNode.addNeighborNode(NeighborType.PREDECESSOR,predecessorNode);
   }

   /**
    * This method removes the edge between the given predecessor and successor if it
    * exists in this DirectedGraph.
    * @param predecessor This is the value that is the starting-point of the edge 
    *                    that will be removed by this method.
    * @param successor This is the value that is the end-point of the edge that will 
    *                  be removed by this method.
    */
   public void removeEdge (final ElementType predecessor, final ElementType successor)
   {
      final Node predecessorNode = nodeMap.get(predecessor);
      if (predecessorNode == null) return;
      final Node successorNode = nodeMap.get(successor);
      if (successorNode == null) return;
      predecessorNode.removeNeighborNode(NeighborType.SUCCESSOR,successorNode);
      successorNode.removeNeighborNode(NeighborType.PREDECESSOR,predecessorNode);
   }

   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;
   }

   /**
    * This method returns the direct successors of the given value in this
    * DirectedGraph.
    * @param value This is the value from which the direct successors are 
    *              returned.
    * @return A List of the direct successors is returned. If value is null
    *         or is not contained in this DirectedGraph null is returned.
    */
   public List<ElementType> getDirectSuccessors (final ElementType value)
   {
      return getDirectNeighbors(NeighborType.SUCCESSOR, value);
   }

   /**
    * This method returns the direct predecessors of the given value in this
    * DirectedGraph.
    * @param value This is the value from which the direct predecessors are 
    *              returned.
    * @return A List of the direct predecessors is returned. If value is null
    *         or is not contained in this DirectedGraph null is returned.
    */
   public List<ElementType> getDirectPredecessors (final ElementType value)
   {
      return getDirectNeighbors(NeighborType.PREDECESSOR, value);
   }

   private List<ElementType> getDirectNeighbors (final NeighborType type,
                                                 final ElementType value)
   {
      assert(type != null);
      final Node node = nodeMap.get(value);
      if (node == null) return null;
      return getValuesFromNodes(node.getNeighborNodes(type));
   }

   /**
    * This method returns all direct and indirect successors of the given 
    * value in this DirectedGraph.
    * @param value This is the value from which the successors are 
    *              returned.
    * @return A List of all successors is returned. If value is null
    *         or is not contained in this DirectedGraph null is returned.
    */
   public List<ElementType> getAllSuccessors (final ElementType value)
   {
      return getAllNeighbors(NeighborType.SUCCESSOR, value);
   }

   /**
    * This method returns all direct and indirect predecessors of the given 
    * value in this DirectedGraph.
    * @param value This is the value from which the predecessors are 
    *              returned.
    * @return A List of all predecessors is returned. If value is null
    *         or is not contained in this DirectedGraph null is returned.
    */
   public List<ElementType> getAllPredecessors (final ElementType value)
   {
      return getAllNeighbors(NeighborType.PREDECESSOR, value);
   }

   private List<ElementType> getAllNeighbors (final NeighborType type,
                                              final ElementType value)
   {
      assert(type != null);
      final Node node = nodeMap.get(value);
      if (node == null) return null;
      LinkedList<Node> currentNeighbors = node.getNeighborNodes(type);
      assert(currentNeighbors != null);
      final LinkedList<ElementType> neighborList = new LinkedList<ElementType>();
      while (currentNeighbors.size() != 0)
      {
         final LinkedList<Node> nextNeighbors = new LinkedList<Node>();
         for (Node currentNode : currentNeighbors)
         {
            final ElementType currentValue = currentNode.getValue();
            if (neighborList.contains(value)) continue;
            neighborList.add(currentValue);
            nextNeighbors.addAll(currentNode.getNeighborNodes(type));
         }
         currentNeighbors = new LinkedList<Node>();
         for (Node currentNode : nextNeighbors)
         {
            if (currentNeighbors.contains(currentNode)) continue;
            currentNeighbors.add(currentNode);
         }
      }
      return neighborList;
   }

   private class Node
   {
      private final HashMap<NeighborType,LinkedList<Node>> neighbors;
      private final ElementType value;

      public Node (final ElementType value)
      {
         assert(value != null);
         neighbors = new HashMap<NeighborType,LinkedList<Node>> (2,1.0f);
         for (final NeighborType type : NeighborType.values())
         {
            neighbors.put(type,new LinkedList<Node>());
         }
         this.value = value;
      }

      public void addNeighborNode (final NeighborType type, final Node neighbor)
      {
         assert(neighbor != null);
         assert(type != null);
         final LinkedList<Node> list = neighbors.get(type);
         if (list.contains(neighbor)) return;
         list.add(neighbor);
      }

      public void removeNeighborNode (final NeighborType type, final Node neighbor)
      {
         assert(type != null);
         neighbors.get(type).remove(neighbor);
      }

      public LinkedList<Node> getNeighborNodes (final NeighborType type)
      {
         assert(type != null);
         return neighbors.get(type);
      }

      public ElementType getValue()
      {
         assert(value != null);
         return value;
      }
   }

   private class GraphIterator implements Iterator<ElementType>
   {
      private final Iterator<ElementType> keyIterator;
      private ElementType current;

      public GraphIterator ()
      {
         keyIterator = nodeMap.keySet().iterator();
      }

      public boolean hasNext()
      {
         return keyIterator.hasNext();
      }

      public ElementType next()
      {
         current = keyIterator.next();
         assert(current != null);
         return current;
      }

      public void remove()
      {
         if (current == null) throw new IllegalStateException();
         final Node node = nodeMap.get(current);
         assert(node != null);
         keyIterator.remove();
         removeConnections(node);
         current = null;
      }
   }
}

[volkard]

was sollte rauskommen, wenn man abstimmt, ob 637986727 eine primzahl ist. sollte das irgend einen sinn ergeben? kaum! und für guten stil gibt's auch gründe, sehr komplexer natur zum teil.

das wird doch unfug mit dem abstimmen.

besser seid ihr dran, wenn ihr einfach Shade glauben schenkt. ne abstimmung kommt sicher zu schlechteren ergebnissen.

[Gregor]

volkard schrieb:

besser seid ihr dran, wenn ihr einfach Shade glauben schenkt. ne abstimmung kommt sicher zu schlechteren ergebnissen.

Ich sehe den Sinn hier nicht in der Abstimmung, sondern darin, dass man anderen Code sieht, den andere für gut halten. Vielleicht nimmt man dabei aus fremdem Code ein paar gute Ideen mit. Das könnte zumindest der Fall sein, wenn dritte zudem noch sagen, was sie gut und was sie schlecht finden.

[volkard]

Gregor schrieb:

Ich verseteh darunter ein gesundes Mißtrauen den anderen gegenüber, welches sich im Code widerspiegelt. Mit den anderen sind die Nutzer deines Codes gemeint, was auch du sein kannst. Man sollte den Code bespielsweise so schreiben, dass Fehler, die von anderen bei der Benutzung des Codes begangen werden schnell auffallen.

aha. man könnte fast meinen, das entsamme einem guten lehrbuch für c++. sollte ich deinen code daraufjin prüfen?

Das heißt, dass man Parameter auf ihre Gültigkeit überprüft und nicht einfach nach irgendwohin weiterreicht oder mit ihnen rechnet: Wenn du in einem Setter die Gültigkeit des Parameters nicht überprüfst und die Membervariable einfach so auf den gegebenen Wert setzt, dann brauchst du dich nicht zu wundern, wenn irgendwo im Programm ein Fehler auftaucht, den du dir nicht erklären kannst.

ok, das ist ein problem von java. in c++ muss man anders vorgehen. da ist generell der aufrufer dafür zuständig, sich an die spezifikationen zu halten.

[qoute]Defensiver Programmierstil beschränkt sich aber natürlich nicht auf Fehlerchecks, sondern kann auch andere Aspekte beinhalten. Beispielsweise kann es manchmal sinnvoll sein, Kopien von Parametern oder Rückgabewerten anzulegen, damit nicht plötzlich jemand unbeabsichtig außerhalb des Objekts dieses verändert.[/quote]
auch ein privates java-problem.

Weiterhin gehört ein sinnvolles Exception Handling natürlich auch zu einem defensiven Programmierstil.

auch ein privates java-problem.

...und so weiter. Grob gesagt heißt defensiver Programmierstil "so programmieren, dass möglichst wenig Bugs entstehen und, dass Bugs möglichst schnell erkannt und gefunden werden können".

ok. die mittel sind anders. du willst so viele fehler wie möglich zur laufzeit fangen (und manchmal sogar einen vermeiden) und wir möchten so viele fehler wie möglich zu compilezeit vermeiden.

aber das ist ja auch genau der punt, warum ich java nicht mag. laufzeitfehler über laufzeitfehler und nicht die geeigneten sprachmittel, um daraus compilezeitfehler zu machen.

Vielleicht nimmt man dabei aus fremdem Code ein paar gute Ideen mit. Das könnte zumindest der Fall sein, wenn dritte zudem noch sagen, was sie gut und was sie schlecht finden.

naja, ich schau mal drüber. aber ich schau mit den augen eines c++-programmierers jetzt java-code an. ihr werdet sofort sehen, daß das nicht sinnvoll ist. für jede sprache gibt's einen eigenen stil mit eigenen regeln. und obwohl java und c++ so ähnlich ausschauen, sind sie doch im herzen ganz unterschiedlich.

/** 
 * This class describes a directed graph. It is not synchronized. 
 * @param <ElementType> This is the type of the values that can be nodes of 
 *                      the DirectedGraph. 
 * @author Gregor 
 * @version 0.1 
 */

also wäre NodeType korrekt und nicht ElementType? merke: wenn man kommentieren muss, was ein name beschreibt, und das in einem satz schafft, dann ist dringend zu überlgenen, oder der name überhaupt klasse war.

hier gehört übrigens in der tat eine beschreibung hin, was für ne klasse das ist. und was sehe ich? einen scheiss. ok, ein gerichteter graph also. aber wie implkementiert? als inzidentmatrix? die matrix gepackt (zum beisliel die zeilen als double linked lists, oder die ganze matrix als hashtable?) voll verzeigert (sorry, verreferenzt natürlich). ich muss nen groben überblick haben, wie die sache innen aussieht, damit ich sofort weiß, ob ich diese klasse für mein problem einsetzen kann. ist nicht gegeben.

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

   private final HashMap<ElementType,Node> nodeMap;

typen und attribute am anfang. das ist gut. so kann ich mir wenigstens einigermaßen schnell einen überblick erahnen, was los sein wird. allerdings schon unfair gegen die, die weniger als 5 jahre erfahrung im graphengeschäft haben, fürchte ich.

/** 
    * This constructor constructs a new DirectedGraph with the initial capacity of a 
    * HashMap constructed by the default constructor. 
    */ 
   public DirectedGraph () 
   { 
      nodeMap = new HashMap<ElementType,Node> (); 
   }

wer die capacity nicht selber angibt, dem ist sie egal. halte den kommentar ür nutzlos.

/** 
    * This constructor constructs a new DirectedGraph with the given initial capacity. 
    * @param initialCapacity This is initial capacity of the new DirectedGraph. The 
    *                        intitialCapacity may not be negative. 
    */ 
   public DirectedGraph (final int initialCapacity) 
   { 
      if (initialCapacity < 0) throw new IllegalArgumentException("InitialCapacity is negative."); 
      nodeMap = new HashMap<ElementType,Node> (initialCapacity); 
   }

obernutzlos. das alles.
was macht dein if hier. heilige mantra "ich muss jedes argument prüfen". warum? new HashMap<ElementType,Node> (initialCapacity); wird schon prüfen.

ideal wäre es natürlich, einen typen zu haben, der aussagen kann, daß nur positive werte möglich sind. also cardinal oder unsigned oder so.

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; 
   }

warum darf man keine null reinstecken?

aber das ist ja gar nicht das schlimme. das schlimme ist der test, ob das element schon da ist.
wenn du was fehlervermeidendes machen willst, dann mach

public void add (final ElementType value) 
   { 
      if (nodeMap.keySet().contains(value)) throw IrgendNeException("ding schon da"); 
      nodeMap.put (value, new Node (value)); 
   } 
public void addIfNotExists (final ElementType value) 
   { 
      if (!nodeMap.keySet().contains(value)); 
          nodeMap.put (value, new Node (value)); 
      /* kenne put nicht, evtl das hier ohne if */
   }

also der benutzer muss bestimmen, was er haben will, und nicht ihm wird erlaubt, nachher vieleicht mal zu prüfen.

public boolean remove (final Object value) 
   { 
      final Node node = nodeMap.get(value); 
      if (node == null) return false; 
      nodeMap.remove(value); 
      removeConnections(node); 
      return true; 
   }

auch hier würde ich einfach verlangen, daß das zu removende objekt auch drin sein muss. in normalen algorithmen ist das nämlich auch der fall. und es wäre ein zeichen für einen schlimm zu findenenden programmierfehler (einen sogenannten logischen fehler), wenn man beim doppelt-löschen erwischt werden würde.

private List<ElementType> getAllNeighbors (final NeighborType type, 
                                              final ElementType value) 
   { 
      assert(type != null); 
      final Node node = nodeMap.get(value); 
      if (node == null) return null; 
      LinkedList<Node> currentNeighbors = node.getNeighborNodes(type); 
      assert(currentNeighbors != null); 
      final LinkedList<ElementType> neighborList = new LinkedList<ElementType>(); 
      while (currentNeighbors.size() != 0) 
      { 
         final LinkedList<Node> nextNeighbors = new LinkedList<Node>(); 
         for (Node currentNode : currentNeighbors) 
         { 
            final ElementType currentValue = currentNode.getValue(); 
            if (neighborList.contains(value)) continue; 
            neighborList.add(currentValue); 
            nextNeighbors.addAll(currentNode.getNeighborNodes(type)); 
         } 
         currentNeighbors = new LinkedList<Node>(); 
         for (Node currentNode : nextNeighbors) 
         { 
            if (currentNeighbors.contains(currentNode)) continue; 
            currentNeighbors.add(currentNode); 
         } 
      } 
      return neighborList; 
   }

was ist denn hier schief gelaufen?

private class Node 
   { 
      private final HashMap<NeighborType,LinkedList<Node>> neighbors; 
      private final ElementType value;

schade, das hätte ich erne früher gewusst.

neighbors = new HashMap<NeighborType,LinkedList<Node>> (2,1.0f);

verstehe ich nicht. wie hast due diese magic numbers bestimmt?

public ElementType getValue() 
      { 
         assert(value != null); 
         return value; 
      }

aha. nur mal zur neugier: was würde ohne das assert passieren?

aber wie gesagt, für java ist das alles voll ok. ihr sollte ja gar nicht versuchen, die c++-tricks einzubauen.

[Gast]

volkard schrieb:

Das heißt, dass man Parameter auf ihre Gültigkeit überprüft und nicht einfach nach irgendwohin weiterreicht oder mit ihnen rechnet: Wenn du in einem Setter die Gültigkeit des Parameters nicht überprüfst und die Membervariable einfach so auf den gegebenen Wert setzt, dann brauchst du dich nicht zu wundern, wenn irgendwo im Programm ein Fehler auftaucht, den du dir nicht erklären kannst.

ok, das ist ein problem von java. in c++ muss man anders vorgehen. da ist generell der aufrufer dafür zuständig, sich an die spezifikationen zu halten.

Kann ich jetzt nicht nachvollziehen. Wenn ich einer C++ Funktion nen ungültigen Parameter übergebe habe ich genauso ein Problem. Wieso sollte das bitte Java-spezifisch sein?

volkard schrieb:

Defensiver Programmierstil beschränkt sich aber natürlich nicht auf Fehlerchecks, sondern kann auch andere Aspekte beinhalten. Beispielsweise kann es manchmal sinnvoll sein, Kopien von Parametern oder Rückgabewerten anzulegen, damit nicht plötzlich jemand unbeabsichtig außerhalb des Objekts dieses verändert.

auch ein privates java-problem.

Ja, fehlende const-correctness ist vielleicht manchmal schade, aber manchmal sind immutable classes sinnvoll möglich und dann ist das unglaublich godlike. Da kann kein const dagegen anstinken. Insgesamt ist das System von Java aber unsicherer, ja.

volkard schrieb:

Weiterhin gehört ein sinnvolles Exception Handling natürlich auch zu einem defensiven Programmierstil.

auch ein privates java-problem.

Bitte? Kannst du das näher ausführen? Seit wann ist Exception-handling ein Java-Problem? Da kann doch Java nichts dafür, wenn die Stream I/O von C++ auf Wunsch auch Fehlercodes liefert. Und ich sehe auch keinen Vorteil darin. Du musst in C++ genauso auf bestimmte Miserfolge reagieren.
Und falls du auf die checked exceptions anspielst: Ich behaupte, dass die meisten Leute, die sie kritisieren sie einfach nur nicht verstehen. Java hat ein mächtiges Exceptionhandling und mit nested Exceptions kann man einiges machen. Wenn man das verstanden hat und richtig benutzt hat man nicht das geringste Problem damit, sondern ist dankbar.
Falls du was anderes als Java-spezifisches Problem darstellen willst, dann kannst du das vielleicht nochmal auf den Punkt bringen?

ok. die mittel sind anders. du willst so viele fehler wie möglich zur laufzeit fangen (und manchmal sogar einen vermeiden) und wir möchten so viele fehler wie möglich zu compilezeit vermeiden.

Das konnte ich aus Gregor's Post nicht rauslesen. Du kannst Sachen wie ungültige Parameter nicht zur compile-zeit feststellen.

Die meisten deiner Kritikpunkte an Gregor's Code finde ich als Java-Programmierer ok. Vor allem der Test, ob das Element schon drinsteckt ist nicht sinnvoll. In einer Map dürfen doppelte Elemente durchaus vorkommen und wenn es ein Set wäre, dann würde das interne Set das auch schon prüfen. null sollte auch erlaubt sein.

@Gregor:

/** 
    * This constructor constructs a new DirectedGraph with the initial capacity of a 
    * HashMap constructed by the default constructor. 
    */ 
   public DirectedGraph () 
   { 
      nodeMap = new HashMap<ElementType,Node> (); 
   }

/** 
    * This constructor constructs a new DirectedGraph with the given initial capacity. 
    * @param initialCapacity This is initial capacity of the new DirectedGraph. The 
    *                        intitialCapacity may not be negative. 
    */ 
   public DirectedGraph (final int initialCapacity) 
   { 
      if (initialCapacity < 0) throw new IllegalArgumentException("InitialCapacity is negative."); 
      nodeMap = new HashMap<ElementType,Node> (initialCapacity); 
   }

Ich würde die Konstruktoren sich gegenseitig aufrufen lassen. (Auch ne tolle Gelegenheit ein nettes Feature von Java zu demonstrieren ) Also:

public DirectedGraph()
{
    this(DEFAULT_CAPACITY);   // ruft DirectedGraph(DEFAULT_CAPACITY) auf.
}

[Shade Of Mine]

Optimizer schrieb:

volkard schrieb:

Weiterhin gehört ein sinnvolles Exception Handling natürlich auch zu einem defensiven Programmierstil.

auch ein privates java-problem.

Bitte? Kannst du das näher ausführen? Seit wann ist Exception-handling ein Java-Problem? Da kann doch Java nichts dafür, wenn die Stream I/O von C++ auf Wunsch auch Fehlercodes liefert. Und ich sehe auch keinen Vorteil darin. Du musst in C++ genauso auf bestimmte Miserfolge reagieren.

Nur dass du gutes Exceptionhandling in C++ daran erkennst, dass du es nicht siehst
Das ist der kleine Unterschied.
Gutes Exceptionhandling in C++ braucht idR keinen oder besser kaum Code, weil du es gratis bekommst.

[Gregor]

volkard schrieb:

also wäre NodeType korrekt und nicht ElementType? merke: wenn man kommentieren muss, was ein name beschreibt, und das in einem satz schafft, dann ist dringend zu überlgenen, oder der name überhaupt klasse war.

Da ist was dran. Der Name ist hier nicht ideal gewählt. Gebe ich zu.

volkard schrieb:

hier gehört übrigens in der tat eine beschreibung hin, was für ne klasse das ist. und was sehe ich? einen scheiss. ok, ein gerichteter graph also. aber wie implkementiert? als inzidentmatrix? die matrix gepackt (zum beisliel die zeilen als double linked lists, oder die ganze matrix als hashtable?) voll verzeigert (sorry, verreferenzt natürlich). ich muss nen groben überblick haben, wie die sache innen aussieht, damit ich sofort weiß, ob ich diese klasse für mein problem einsetzen kann. ist nicht gegeben.

Ich sehe ein, dass ich vielleicht bestimmte Zusagen bezüglich der Komplexität von Methoden angeben sollte. Ich denke aber nicht, dass ich die Implementierung komplett verraten sollte. Dann würde ich mich diesbezüglich festlegen und darf das nie wieder ändern. Das wäre ja gegen das Design des Codes gerichtet. Naja, ok: Inwiefern es überhaupt realistisch ist, dass sich an dieser Klasse jemals etwas grundlegendes ändert, ist eine ganz andere Sache. Das gebe ich zu.

volkard schrieb:

typen und attribute am anfang. das ist gut. so kann ich mir wenigstens einigermaßen schnell einen überblick erahnen, was los sein wird. allerdings schon unfair gegen die, die weniger als 5 jahre erfahrung im graphengeschäft haben, fürchte ich.

Willst du hier Kommentare haben? Das sind hier Dinge, die nicht zur öffentlichen Schnittstelle der Klasse gehören und einen Nutzer der Klasse nichts angehen. Wer sich damit auseinandersetzt, der will die Klasse selbst verändern und sollte dann auch das nötige Wissen hierzu mitbringen und den Code der Klasse komplett verstehen, bevor er etwas ändert. Ich denke, dass Kommentare da nicht allzu wichtig sind. Kann man aber natürlich auch anders sehen.

volkard schrieb:

/** 
    * This constructor constructs a new DirectedGraph with the given initial capacity. 
    * @param initialCapacity This is initial capacity of the new DirectedGraph. The 
    *                        intitialCapacity may not be negative. 
    */ 
   public DirectedGraph (final int initialCapacity) 
   { 
      if (initialCapacity < 0) throw new IllegalArgumentException("InitialCapacity is negative."); 
      nodeMap = new HashMap<ElementType,Node> (initialCapacity); 
   }

obernutzlos. das alles.
was macht dein if hier. heilige mantra "ich muss jedes argument prüfen". warum? new HashMap<ElementType,Node> (initialCapacity); wird schon prüfen.

1. Kommentar: Nicht überflüssig, da Parameternamen in class-Dateien nicht vorhanden sind und man manchmal nur class-Dateien (und vielleicht Doku) zur Verfügung hat. Da hast du dann als Signatur dieses Construktors nur folgendes public DirectedGraph(int). Sehe ich als wirkliche Schwachstelle von Java an. In dem Fall ist es zumindest gut, wenn man in der Doku einen Hinweis darauf hat, um was es sich bei dem int handeln könnte. Bei mehreren Parametern gibt es da natürlich auch Probleme, ist aber besser als gar nichts. Abgesehen davon ist die Doku ohne diese Beschreibung einfach nicht komplett. Ist ne Frage der Ästhetik: Ein Javadoc-html-File mit leeren Kästchen ist einfach nicht angebracht. Ein Hinweis darauf, dass der Parameter nicht negativ sein darf ist auch angebracht. Ich könnte mit negativen Werten ja auch anders umgehen und sie zum Beispiel einfach auf 0 setzen oder so.
2. Genau: Das heilige Mantra. Ich prüfe grundsätzlich alles. Ich möchte nicht erst durch 5 Klassen navigieren, um zur Fehlerquelle zu gelangen. Die soll gefälligst möglichst nah an der Stelle liegen, an der ich zuerst nachgucke. BTW: Vielleicht geht die Hashmap ja auch ganz anders mit negativen Anfangskapazitäten um und schmeißt dann keine Exception. Ich weiß das gar nicht, aber ich möchte mich nicht darauf verlassen, zumal ich auf die Idee kommen könnte, die Implementierung der Klasse zu ändern und die Hashmap durch etwas anderes zu ersetzen. Dann habe ich da plötzlich etwas anderes stehen, habe aber nicht daran gedacht, den Check wieder einzubauen.

volkard schrieb:

ideal wäre es natürlich, einen typen zu haben, der aussagen kann, daß nur positive werte möglich sind. also cardinal oder unsigned oder so.

Ja, hast Recht. Es gibt in jeder Sprache Dinge, an denen man vorbeidesignen muss.

volkard schrieb:

warum darf man keine null reinstecken?

Weil ich es verbiete. Abgesehen davon funktioniert null nicht als Schlüssel einer HashMap in Java. ...AFAIK.

volkard schrieb:

aber das ist ja gar nicht das schlimme. das schlimme ist der test, ob das element schon da ist.

Der ist absolut wichtig, weil ohne den Test jede Kante von und zu dem Knoten durch das "new Node()" gelöscht werden würde, wenn ein schon vorhandener Knoten nochmal hinzugefügt wird. Ich gebe dir aber Recht, dass man da auch eine Exception schmeißen könnte. Ich habe mich in diesem Fall anders entschieden. Nicht zuletzt aus dem Grund, dass dieses Verhalten mehr oder weniger durch die Schnittstelle vorgeschrieben wird. Gleiches gilt für die remove-Methode.

volkard schrieb:

was ist denn hier schief gelaufen?

Steck deinen salzigen Finger hier am Besten mal richtig tief in die Wunde. Was du da siehst ist neben einem Umgehen einer Java-Eigenart pures Unvermögen, vernünftigen Code produzieren zu können. Ich habe bei dieser Methode echt ein schlechtes Gewissen.

volkard schrieb:

neighbors = new HashMap<NeighborType,LinkedList<Node>> (2,1.0f);

verstehe ich nicht. wie hast due diese magic numbers bestimmt?

Die sind eigentlich komplett überflüssig und dienen nur dazu, den Speicher etwas zu schonen. Durch die 1.0 gebe ich an, dass die HashMap voll sei soll, bevor sie erweitert wird, durch die 2 gebe ich an, dass voll=2 Elemente bedeutet. In diesem Fall sind es 2 Listen: Eine für die Vorgänger, eine für die Nachfolger des Knotens. Hätte ich aber benennen können und sollen. Gebe ich zu.

volkard schrieb:

public ElementType getValue() 
      { 
         assert(value != null); 
         return value; 
      }

aha. nur mal zur neugier: was würde ohne das assert passieren?

Erstmal genau das Gleiche. Assert muss man in Java extra anschalten. Sonst existiert es einfach nicht. ...kostet dann auch keine Performance. Checks, die nicht die öffentliche Schnittstelle der Klasse betreffen, sich also im inneren von Methoden bzw. im inneren der Klasse befinden, macht man in Java normalerweise mit assert. Ist eigentlich nur zum debuggen gedacht, um bei einem Fehler innerhalb der Klasse näher an die Fehlerquelle heranzukommen. Ich habe bereits gute Erfahrungen damit gemacht. Also, als Faustregel gilt in Java: Mit Exceptions reagiert man auf Fehler anderer, mit assert auf die eigenen.