Was ist für euch guter Programmcode?

[Walli]

Artchi schrieb:

Gregor! In C++ returned er eine Kopie. Und eine Reference zurück geben, macht man nicht (auch wenn es möglich ist).

Wieso? Man kann doch eine const Reference zurück geben wenn der Benutzer nur lesen darf.

[SideWinder]

Artchi schrieb:

Gregor! In C++ returned er eine Kopie. Und eine Reference zurück geben, macht man nicht (auch wenn es möglich ist).

Wie baust du op=?

MfG SideWinder

[Mecnels]

Prinzipiell stehen in meinen Quellcodes immer zuerst die
Kommentare da, und erst danach schreibe ich den korres-
pondierenden C++ Code nieder. Das sieht dann z.B. so aus:

void MTactics::Computer_zieht(){

	// Zuerst werden alle Zuege im hilfsvektorarray auf (-1,-1)
	// gesetzt, um zu signalisieren, dass noch kein moeglicher
	// Zug gefunden wurde
	short i,j,k,m; // Allenfalls benoetigte Schleifenvariablen
	short indextabelle[10];
	UINT prinzmoe;
	Vektor helper(-1,-1);
	Vektor analysiert(0,0);
	for(i=0;i<210;i++){
		hilfsvektorarray[0][i]=helper;
	}

	m=0; // Diese Variable speichert den aktuellen Arrayindex im hilfsvektorarray

	// Prinzipiell schaut das jetzt so aus:
	// a) Klappere das Spielfeld von links oben nach rechts unten
	//	  ab, falls du einen Stein entdeckst, der Dir gehoert DANN

	for(i=0;i<10;i++){
		for(j=0;j<10;j++){
			if(Spielfeld[j][i].aktuell!=0){ // um nicht auf
											// Nullzeiger zuzugreifen
				if(Spielfeld[j][i].Hiesige_Steine[0]->besitzer==2){
	// b) Frage die Anzahl der mit diesem Stein prinzipiell moeg-
	//    lichen Zuege ab (im Zugvektorarray des Steins gespeichert)
					prinzmoe=Spielfeld[j][i].Hiesige_Steine[0]->Zuegearray->GetZuegezahl();
	// c) Fuer jeden prinzipiell moeglichen Zug rufe die Funktion
	//	  ZugMoeglich(Zielfeld,Spielstein) auf
					for(k=1;k<=prinzmoe;k++){
						analysiert=Spielfeld[j][i].Hiesige_Steine[0]->Zuegearray->GetZug(k);
						Startfeld.x=j; // Die Funktionen ZugMoeglich und ExecuteMove benoetigen
						Startfeld.y=i; // ein gut initialisiertes Startfeld
						if(ZugMoeglich(Startfeld+analysiert,Spielfeld[j][i].Hiesige_Steine[0])){
						// d) Falls der Zug auch dann noch moeglich ist, landen die
						//	  Koordinaten des Startfeldes (also die momentanen Werte
						//	  der Schleifenvariablen) im hilfsvektorarray, ebenso
						//	  die Koordinaten des Zielfeldes (Startfeld + gefundener
						//	  moeglicher Zug)
						//		NICHT VERGESSEN - c) und d) sind fuer jeden unter b)
						//		gefundenen prinzipiell moeglichen Zug durchzufuehren
						hilfsvektorarray[0][m]=Startfeld;
						hilfsvektorarray[1][m]=Startfeld+analysiert;
						m++; // bis zum naechsten gefundenen moeglichen Zug
						}
					}
				}
			}
		}
	}	// Schliessende Klammer der for-Schleife mit i

	// e) Nach Durchlaufen der Schleifenkonstruktion a) bis d)
	//	  befinden sich im hilfsvektorarray wahrscheinlich eine
	//	  ganze Menge moeglicher Start und Zielfelder, wobei
	//	  das erste Auftreten von (-1,-1) den ersten undurch-
	//	  fuehrbaren Zugindex signalisiert
	//	  uebrigens, die Variable m muesste ja jetzt eigentlich die Anzahl der
	//	  gefundenen moeglichen Zuege enthalten!!! Falls 0, kam offenbar kein
	//	  einziger moeglicher Zug zustande und wir koennen sofort zu Spielphase 7
	//	  (Spielende) gehen!!!
	if(m==0){
		SendMessage(Startbutton,WM_SETTEXT,0,(LPARAM)"New");
		MessageBox(NULL,"You win (Found no regular move for Computer)","Game over",0);
		Spielphase=7;
		return;
	} // Andernfalls kommen wir jetzt schon zur Ausfuehrung des Zuges

usw usf.

Wie leicht zu sehen ist war ich hier gerade vor dem Problem, einem Programm beizubringen, wie es herausfindet, welche Züge es mit seinen Spielsteinen
bei einem Brettspiel machen kann. Gute Kommentierung und über sich selber
sprechende Bezeichner machen für mich einen gut leserlichen Quellcode aus.
Der Vorteil ist einfach der, wenn man z.B. ein Programm für ein paar
Wochen zur Seite legt und dann wieder versucht, es zu erweitern, hilft eine
gute Kommentierung einem vor allem auch selber, sich im eigenen Programm aus-
zukennen.
Netten Abend noch

[Shade Of Mine]

Gregors Beispiel ist super, auch direkt in C++ übertragbar, nur ist dort die Problematik nicht so groß:
liefere nie eine Referenz auf interne Daten zurück
heißt hier der Merksatz

aber es steht ja generell dafür, dem User mehr zu erlauben als er eigentlich braucht. Dazu zählen auch set-Methoden die keine Werte checken, sondern blind den übergebenen Wert der internen Variablen zuweisen.

Denn dann ist die interne Variable ja quasi public.
Deshalb anderer Merksatz:
erstelle getter/setter mit bedacht
und
setter die nix checken, sollten nochmal überdacht werden (ist wirklich jeder wert valid)

letzter merksatz gilt für viele valuetypes nicht.

[borg]

Mecnels schrieb:

short i,j,k,m; // Allenfalls benoetigte Schleifenvariablen
[...]
m=0; // Diese Variable speichert den aktuellen Arrayindex im hilfsvektorarray
[...]
}    // Schliessende Klammer der for-Schleife mit i

oha.. schreib die erklärungen mit a), b), c) usw oberhalb der funktion. den ganzen rest mit "diese variable beinhaltet dies, jene klammer macht das" lässt du weg. dann initialisierst du schleifenvariablen erst wenn du sie brauchst und schon hat dein code um einiges an elleganz gewonnen

edit: und was zum geier ist ein "hilfsvektorarray"?

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