Was ist für euch guter Programmcode?

BloodLord

www.ioccc.org konnte nicht gefunden werden...

Un sowieso man schreibt doch nicht plötzlich einfach so was in nen monate alten Flamethread rein...

Konrad

BloodLord schrieb:

www.ioccc.org konnte nicht gefunden werden...

Un sowieso man schreibt doch nicht plötzlich einfach so was in nen monate alten Flamethread rein...

Warum nicht?

lang. schrieb:

ist sowas guter code: http://rafb.net/paste/results/LLUB3v51.html ?

leider sehen alle mir bekannten open source server so aus. da braucht man sich über sicherheitslöcher nicht zu wundern.

volkard

http schrieb:

lang. schrieb:

ist sowas guter code: http://rafb.net/paste/results/LLUB3v51.html ?

leider sehen alle mir bekannten open source server so aus. da braucht man sich über sicherheitslöcher nicht zu wundern.

lol. der code ist cool.

hab mal ein wenig den von lightppd gelesen, um ein bug zu fixen. war eigentlich ganz lesbar un den gegenden, wo ich stöbern mußte. (aber noch weit von "hübsch" entfernt.)

BloodLord

Immer noch schöner als mein Code

Mir fällt gerade so nebenbei ein: Gibt es eigentlich einen Standard für das Zählen von Zeilennummern eines Programmes? Oft gibt man ja an "Core: 100.000 Lines" oder so ähnliches. Die könneten jetzt aber so:

1: #include <iostream>
2: #include <string>
3: using namespace std;
4: int main() { cout << "Welcome!" << endl; string eingabe; cin.getline(eingabe,80); cout << eingabe; return 0; }

oder so aussehn:

1:         /*¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯/
 2:        /        The genius Input-Output Tool       /
 3:       /         ============================      / 
 4:      /                                           /
 5:     /   Version 0.1                             /
 6:    /  (C) 2005 by whoever                      /
 7:   /__________________________________________*/
 8:
 9:
10:
11: // Include section
12: 
13:   #include <iostream>
14:
15:      /* Includes iostream standard library, required for
16:         using cin and cout methods.             */
17:
18:   #include <string>
19
20:      /* Includes string standard library        */
21:
22: // End of include Section
23:
24:
25: using namespace std; // Sets namespace std
26:
27:
28:
29: int main()
30:
31: /* Function main...
32:          (stupid discription) ...           */
33: {
34:
35:     cout << "Welcome!"
36:          << endl;
37:
38:
39:     string eingabe;
40:
41:
42:     cin.getline(eingabe,80);
43:       
44:     cout << eingabe;
45:
46:
47:     return 0;
48:
49: }

Walli

Meist gibt man effective lines of code an. Da werden Kommentare, Leerzeilen usw. nicht mitgezählt.

volkard

BloodLord schrieb:

Mir fällt gerade so nebenbei ein: Gibt es eigentlich einen Standard für das Zählen von Zeilennummern eines Programmes?

ja. man zählt die nur zeilenumbrüche und tut dann so, als gäbe es pro zeilenumbruch eine zeile.

Oft gibt man ja an "Core: 100.000 Lines" oder so ähnliches.

in meinen kreisen sagt sowas keiner. und wenn, würden die anderen ihn mitleidig belächeln.
wenn einer grade freundlich drauf ist, sagt er vielleicht noch "geht das nicht auch in 5000 Lines?".

Die könneten jetzt aber so:
oder so aussehn:

die lange version macht man, wenn man nach zeilen bezahlt wird. hat nur den effekt, daß mit der zeit alle mehr zeilen machen müssen (weniger kohle pro zeile kriegen) und ordenlicher code nicht mehr gebaut werden kann.

volkard schrieb:

http schrieb:

lang. schrieb:

ist sowas guter code: http://rafb.net/paste/results/LLUB3v51.html ?

leider sehen alle mir bekannten open source server so aus. da braucht man sich über sicherheitslöcher nicht zu wundern.

lol. der code ist cool.

Was meinst du mit cool?

volkard schrieb:

http schrieb:

lang. schrieb:

ist sowas guter code: http://rafb.net/paste/results/LLUB3v51.html ?

leider sehen alle mir bekannten open source server so aus. da braucht man sich über sicherheitslöcher nicht zu wundern.

lol. der code ist cool.

hab mal ein wenig den von lightppd gelesen, um ein bug zu fixen. war eigentlich ganz lesbar un den gegenden, wo ich stöbern mußte. (aber noch weit von "hübsch" entfernt.)

lol. die http request parse funktion ist dort ja noch länger. 739 zeilen. das kann doch echt nicht wahr sein.

http://www.rafb.net/paste/results/96dVAS88.html

volkard

http schrieb:

lol. die http request parse funktion ist dort ja noch länger. 739 zeilen. das kann doch echt nicht wahr sein.

ok. ich nehme den nicht mehr.

volkard schrieb:

http schrieb:

lol. die http request parse funktion ist dort ja noch länger. 739 zeilen. das kann doch echt nicht wahr sein.

ok. ich nehme den nicht mehr.

thx

ich freue mich schon auf deinen.

BloodLord schrieb:

Un sowieso man schreibt doch nicht plötzlich einfach so was in nen monate alten Flamethread rein...

Ich fand die Dinge, die in diesem Thread gesagt wurden teilweise sehr interessant. Ich sehe den somit auch nicht als "Flamethread" an.

BTW: Da der Thread jetzt schon reanimiert wurde, kann ich ja wiedermal'n bischen Code posten, gegen den man dann flamen kann.

Das ist Code, bei dem ich mir eigentlich keine besonders große Mühe gemacht habe. Ich halte es also nicht für besonders guten Code. Es fehlen auch Kommentare usw.. Ich bin mal gespannt, was da für Verbesserungsvorschläge kommen, falls dazu noch einer Lust hat.

Der Code ist letztendlich eine Realisierung eines n-dimensionalen Bresenham-Linienalgorithmus inklusive einer Clipping-Funktion, die dem Liang-Barsky Algorithmus nahe kommt. Es wird allerdings keine Linie gemalt, sondern "analysiert".

/*
 * LineCaster.java
 *
 * Created on 8. März 2005, 01:25
 */

package jaradap.model.algorithm;

import jaradap.model.algorithm.multiPointProcessor.RaycastProcessor;
import jaradap.model.data.BasicRaster;
import jaradap.model.data.StandardRaster;
import math.linearAlgebra.Vector;

/**
 *
 * @author Gregor
 */
public class LineCaster
{
   private final RaycastProcessor pointProcessor;
   private final StandardRaster raster;
   private final int dimension;

   /** Creates a new instance of RayCaster */
   public LineCaster(final RaycastProcessor pointProcessor, final StandardRaster raster)
   {
      this.pointProcessor = pointProcessor;
      this.raster = raster;
      dimension = raster.getDimensions();
   }

   public float castLine(final Vector startPoint, final Vector endPoint)
   {
      if (startPoint.getDimension() != dimension) 
         throw new IllegalArgumentException("startPoint has wrong dimension.");
      if (endPoint.getDimension() != dimension) 
         throw new IllegalArgumentException("endPoint has wrong dimension.");
      final int[] realStartPoint = new int[dimension];
      final int[] realEndPoint = new int[dimension];
      if(!setFirstPointInRaster(realStartPoint,startPoint,endPoint)) return 0.0f;
      if(!setFirstPointInRaster(realEndPoint,endPoint,startPoint)) return 0.0f;
      int maxDiffIndex = 0;
      final int[] intDifferences = new int[dimension];
      final int[] relativePosition = new int[dimension];
      final int[] position = new int[dimension];
      final int[] increments = new int[dimension];
      for(int i = 0 ; i < dimension ; ++i)
      {
         intDifferences[i] = realEndPoint[i] - realStartPoint[i];
         increments[i] = (intDifferences[i] > 0) ? 1 : -1;
         intDifferences[i] = Math.abs(intDifferences[i]);
         if (intDifferences[i] > intDifferences[maxDiffIndex]) maxDiffIndex = i;
         relativePosition[i] = intDifferences[i] >> 1;
         position[i] = realStartPoint[i];
      }
      final int maxDifference = intDifferences[maxDiffIndex];
      final int maxIncrement = increments[maxDiffIndex];
      pointProcessor.reset();
      for(;position[maxDiffIndex] != realEndPoint[maxDiffIndex] ; position[maxDiffIndex] += maxIncrement)
      {
         if (pointProcessor.addPoint(raster,position)) break;
         for(int i = 0 ; i < dimension ; ++i)
         {
            if (i == maxDiffIndex) continue;
            relativePosition[i] += intDifferences[i];
            if (relativePosition[i] < maxDifference) continue;
            position[i] += increments[i];
            relativePosition[i] -= maxDifference;
         }
      }
      pointProcessor.addPoint(raster,position);
      return pointProcessor.getValue();
   }

   private final boolean setFirstPointInRaster(final int[] target, final Vector firstLinePoint, 
                                               final Vector lastLinePoint)
   {
      final double[] differences = new double[dimension];
      double fraction = 0.0;
      for (int i = 0 ; i < dimension ; ++i)
      {
         differences[i] = (float)(lastLinePoint.getComponent(i) - firstLinePoint.getComponent(i));
      }
      for (int i = 0 ; i < dimension ; ++i)
      {
         double newFraction = 0.0;
         final double component = firstLinePoint.getComponent(i);
         final double difference = differences[i];
         final double maxValue = raster.getSize(i) - 1;
         if (component < 0.0)
         {
            if (difference <= 0.0) return false;
            newFraction = component / difference;
         }
         else if (component > maxValue)
         {
            if (difference >= 0.0) return false;
            newFraction = (component - maxValue) / difference;
         }
         if (newFraction < fraction) fraction = newFraction;
      }
      for (int i = 0 ; i < dimension ; ++i)
      {
         target[i] = Math.round((float)(firstLinePoint.getComponent(i) - fraction * differences[i]));
         if (target[i] < 0) return false;
         if (target[i] > raster.getSize(i) - 1) return false;
      }
      return true;
   }
}

IMHO: Viel zu lange Funktionen mit zu vielen lokalen Variablen. Da sollte man versuchen, besser aufzuteilen. Zwing mich jetzt aber nicht, darüber nachzudenken, wo.

Gregor@Home schrieb:

Das ist Code, bei dem ich mir eigentlich keine besonders große Mühe gemacht habe. Ich halte es also nicht für besonders guten Code. Es fehlen auch Kommentare usw.. Ich bin mal gespannt, was da für Verbesserungsvorschläge kommen, falls dazu noch einer Lust hat.

Naja, um es nochmal anders auszudrücken: Ich habe da einen Algorithmus implementiert, der IMHO nicht so ganz leicht zu implementieren ist. Es war schwer für mich, den Code überhaupt zu produzieren, so dass ich nicht so auf "guten Stil" geachtet habe. Das ist sicherlich eine Frage der Erfahrung, aber mich interessiert, ob es euch bei schwierigen Algorithmen ähnlich geht:

Wird bei euch der Stil bei schwierigen Programmieraufgaben schlechter oder produziert ihr unabhängig von der Komplexität der Aufgabe immer gleichbleibend guten Code?

volkard

Gregor@Home schrieb:

Wird bei euch der Stil bei schwierigen Programmieraufgaben schlechter oder produziert ihr unabhängig von der Komplexität der Aufgabe immer gleichbleibend guten Code?

der wird zunächst signifikant schlechter. auch mal schlecht, daß ich den überblick verliere und löschen muss. hab gerade wieder tagelang so ne phase gehabt. jetzt geht's langsam wieder.

volkard schrieb:

der wird zunächst signifikant schlechter. auch mal schlecht, daß ich den überblick verliere und löschen muss. hab gerade wieder tagelang so ne phase gehabt. jetzt geht's langsam wieder.

Führst du dann Refactorings durch, bis die Qualität des Codes der deines restlichen Codes entspricht?

Optimizer schrieb:

IMHO: Viel zu lange Funktionen mit zu vielen lokalen Variablen. Da sollte man versuchen, besser aufzuteilen. Zwing mich jetzt aber nicht, darüber nachzudenken, wo.

Ich habe mal angefangen, da ein bischen was aufzuteilen, aber irgendwie habe ich das Gefühl, dass das so nicht wirklich etwas wird. Ich habe plötzlich Methoden, die unglaublich viele Parameter brauchen und die Länge der Klasse hat sich auch mal eben um 30 Zeilen erhöht.

da ist der Code:

/*
 * LineCaster.java
 *
 * Created on 8. März 2005, 01:25
 */

package jaradap.model.algorithm;

import jaradap.model.algorithm.multiPointProcessor.RaycastProcessor;
import jaradap.model.data.BasicRaster;
import jaradap.model.data.StandardRaster;
import math.linearAlgebra.Vector;

/**
 *
 * @author Gregor
 */
public class LineCaster
{
   private final RaycastProcessor pointProcessor;
   private final StandardRaster raster;
   private final int dimension;

   /** Creates a new instance of RayCaster */
   public LineCaster(final RaycastProcessor pointProcessor, final StandardRaster raster)
   {
      this.pointProcessor = pointProcessor;
      this.raster = raster;
      dimension = raster.getDimensions();
   }

   public float castLine(final Vector startPoint, final Vector endPoint)
   {
      checkCastLineParameters(startPoint, endPoint);
      final int[][] line = clipLine(startPoint, endPoint);
      if (line == null) return 0.0f;
      final int[] intDifferences = new int[dimension];
      final int[] relativePosition = new int[dimension];
      final int[] position = new int[dimension];
      final int[] increments = new int[dimension];
      final int maxDiffIndex = initHelperVariables(intDifferences, relativePosition, position, 
                                                   increments, line);
      final int maxDifference = intDifferences[maxDiffIndex];
      final int maxIncrement = increments[maxDiffIndex];
      pointProcessor.reset();
      processLine(pointProcessor, intDifferences, relativePosition, position, increments, 
                  maxDiffIndex, maxDifference, maxIncrement, line);
      return pointProcessor.getValue();
   }

   private final void checkCastLineParameters(final Vector startPoint, final Vector endPoint)
   {
      if (startPoint.getDimension() != dimension) 
         throw new IllegalArgumentException("startPoint has wrong dimension.");
      if (endPoint.getDimension() != dimension) 
         throw new IllegalArgumentException("endPoint has wrong dimension.");
   }

   private final int[][] clipLine(final Vector startPoint, final Vector endPoint)
   {
      final int[][] line = new int[2][dimension];
      if(!setFirstPointInRaster(line[0],startPoint,endPoint)) return null;
      if(!setFirstPointInRaster(line[1],endPoint,startPoint)) return null;
      return line;
   }

   private final int initHelperVariables(final int[] intDifferences, final int[] relativePosition,
                                         final int[] position, final int[] increments,
                                         final int[][] line)
   {
      int maxDiffIndex = 0;
      for(int i = 0 ; i < dimension ; ++i)
      {
         intDifferences[i] = line[1][i] - line[0][i];
         increments[i] = (intDifferences[i] > 0) ? 1 : -1;
         intDifferences[i] = Math.abs(intDifferences[i]);
         if (intDifferences[i] > intDifferences[maxDiffIndex]) maxDiffIndex = i;
         relativePosition[i] = intDifferences[i] >> 1;
         position[i] = line[0][i];
      }
      return maxDiffIndex;
   }

   private final void processLine(final RaycastProcessor pointProcessor,final int[] intDifferences, 
                                  final int[] relativePosition, final int[] position, 
                                  final int[] increments, final int maxDiffIndex, 
                                  final int maxDifference, final int maxIncrement,
                                  final int[][] line)
   {
      for(;position[maxDiffIndex] != line[1][maxDiffIndex] ; position[maxDiffIndex] += maxIncrement)
      {
         if (pointProcessor.addPoint(raster,position)) break;
         for(int i = 0 ; i < dimension ; ++i)
         {
            if (i == maxDiffIndex) continue;
            relativePosition[i] += intDifferences[i];
            if (relativePosition[i] < maxDifference) continue;
            position[i] += increments[i];
            relativePosition[i] -= maxDifference;
         }
      }
      pointProcessor.addPoint(raster,position);
   }

   private final boolean setFirstPointInRaster(final int[] target, final Vector firstLinePoint, 
                                               final Vector lastLinePoint)
   {
      final double[] differences = new double[dimension];
      double fraction = 0.0;
      for (int i = 0 ; i < dimension ; ++i)
      {
         differences[i] = (float)(lastLinePoint.getComponent(i) - firstLinePoint.getComponent(i));
      }
      for (int i = 0 ; i < dimension ; ++i)
      {
         double newFraction = 0.0;
         final double component = firstLinePoint.getComponent(i);
         final double difference = differences[i];
         final double maxValue = raster.getSize(i) - 1;
         if (component < 0.0)
         {
            if (difference <= 0.0) return false;
            newFraction = component / difference;
         }
         else if (component > maxValue)
         {
            if (difference >= 0.0) return false;
            newFraction = (component - maxValue) / difference;
         }
         if (newFraction < fraction) fraction = newFraction;
      }
      for (int i = 0 ; i < dimension ; ++i)
      {
         target[i] = Math.round((float)(firstLinePoint.getComponent(i) - fraction * differences[i]));
         if (target[i] < 0) return false;
         if (target[i] > raster.getSize(i) - 1) return false;
      }
      return true;
   }
}

Findest du, der Code hat sich verbessert?

Man könnte das jetzt natürlich noch weiter treiben und zum Beispiel eine neue Klasse einführen, in der die ganzen Hilfsvariablen Member sind. So könnte man sicherlich die vielen Parameter bei den Methodenaufrufen verhindern. Aber letztendlich landet man dann vermutlich irgendwann bei einem Monster mit 1000 Zeilen, wo es auch 100 getan hätten. Zudem dürfte der Code durch solche "Verbesserungen" immer träger/langsamer werden. Ich glaube nicht, dass dieser Weg der Beste ist. Fragt sich nur, wie man den Code ansonsten verbessern sollte, denn der Ausgangscode ist ja immer noch nicht schöner geworden.

Oder habe ich mich bei der Aufteilung der Methoden einfach nur sehr blöd angestellt? Geht das vielleicht deutlich besser?

(Ätsch, jetzt mußt du doch denken. )

SideWinder

Irgendwie fehlt dem armen Side ein Kommentar wo was steht bzgl. was LineCaster sein soll Überhaupt sehen alle Kommentare ziemmlich auto-eclipse-created aus.

MfG SideWinder

SideWinder schrieb:

Irgendwie fehlt dem armen Side ein Kommentar wo was steht bzgl. was LineCaster sein soll Überhaupt sehen alle Kommentare ziemmlich auto-eclipse-created aus.

Ja, das ist sicherlich ein treffender Punkt. Vor Kommentaren habe ich mich in dieser Klasse bisher etwas gedrückt, dabei würden die sicherlich helfen. Vielleicht kann man damit auch innerhalb von Methoden verschiedene Bereiche markieren und kann sich so eine Aufteilung der Methoden sparen.

Was die Klasse prinzipiell macht, habe ich etwas weiter oben geschrieben:

Der Code ist letztendlich eine Realisierung eines n-dimensionalen Bresenham-Linienalgorithmus inklusive einer Clipping-Funktion, die dem Liang-Barsky Algorithmus nahe kommt. Es wird allerdings keine Linie gemalt, sondern "analysiert".

Ich habe mal mein jämmerliches Englisch aktiviert, um die Klasse halbwegs zu kommentieren. Was sagt ihr dazu:

/*
 * LineCaster.java
 *
 * Created on 8. März 2005, 01:25
 */

package jaradap.model.algorithm;

import jaradap.model.algorithm.multiPointProcessor.RaycastProcessor;
import jaradap.model.data.BasicRaster;
import jaradap.model.data.StandardRaster;
import math.linearAlgebra.Vector;

/**
 * This class represents Objects that can analyze lines in a given raster. The analysis
 * projects a line into a single float which is given by a RaycastProcessor, the raster and the line. 
 *
 * @author Gregor
 */
public class LineCaster
{
   private final RaycastProcessor pointProcessor;
   private final StandardRaster raster;
   private final int dimension;

   /** 
    * Creates a new LineCaster with the given parameters.
    *
    * @param pointProcessor This is the RaycastProcessor that will be used to analyze the lines 
    *                       in the method castLine.
    * @param raster This is the raster which represents the environment in that this LineCaster
    *               may analyze lines.
    */
   public LineCaster(final RaycastProcessor pointProcessor, final StandardRaster raster)
   {
      this.pointProcessor = pointProcessor;
      this.raster = raster;
      dimension = raster.getDimensions();
   }

   /**
    * This method analyzes the line which is specified by the parameters. The pointProcessor
    * of this LineCaster is used to compute the resulting value, while the raster of this LineCaster
    * is the environment of the analysis.
    *
    * @param startPoint This is the start point of the line.
    * @param endPoint This is the end point of the line.
    * @return The value of the pointProcessor after processing all points of the line inside the raster
    *         will be returned. If the line has no points inside the raster 0 will be returned.
    */
   public float castLine(final Vector startPoint, final Vector endPoint)
   {
      // Check parameters.
      if (startPoint.getDimension() != dimension)
         throw new IllegalArgumentException("startPoint has wrong dimension.");
      if (endPoint.getDimension() != dimension)
         throw new IllegalArgumentException("endPoint has wrong dimension.");

      // Clip line: 
      // Resulting line is given by rasterStartPoint and rasterEndPoint.
      final int[] rasterStartPoint = new int[dimension];
      final int[] rasterEndPoint = new int[dimension];
      if(!setFirstPointInRaster(rasterStartPoint,startPoint,endPoint)) return 0.0f;
      if(!setFirstPointInRaster(rasterEndPoint,endPoint,startPoint)) return 0.0f;

      // Init helper variables.
      int maxDiffIndex = 0;
      final int[] intDifferences = new int[dimension];
      final int[] relativePosition = new int[dimension];
      final int[] position = new int[dimension];
      final int[] increments = new int[dimension];
      for(int i = 0 ; i < dimension ; ++i)
      {
         intDifferences[i] = rasterEndPoint[i] - rasterStartPoint[i];
         increments[i] = (intDifferences[i] > 0) ? 1 : -1;
         intDifferences[i] = Math.abs(intDifferences[i]);
         if (intDifferences[i] > intDifferences[maxDiffIndex]) maxDiffIndex = i;
         relativePosition[i] = intDifferences[i] >> 1;
         position[i] = rasterStartPoint[i];
      }
      final int maxDifference = intDifferences[maxDiffIndex];
      final int maxIncrement = increments[maxDiffIndex];

      // Process line: 
      // 1. Reset pointProcessor.
      // 2. Process Bresenham-Line-Algorithm
      // 3. Return value of pointProcessor
      pointProcessor.reset();
      for(;position[maxDiffIndex] != rasterEndPoint[maxDiffIndex] ; position[maxDiffIndex] += maxIncrement)
      {
         if (pointProcessor.addPoint(raster,position)) break;
         for(int i = 0 ; i < dimension ; ++i)
         {
            if (i == maxDiffIndex) continue;
            relativePosition[i] += intDifferences[i];
            if (relativePosition[i] < maxDifference) continue;
            position[i] += increments[i];
            relativePosition[i] -= maxDifference;
         }
      }
      pointProcessor.addPoint(raster,position);
      return pointProcessor.getValue();
   }

   private final boolean setFirstPointInRaster(final int[] target, final Vector firstLinePoint,
                                               final Vector lastLinePoint)
   {
      // Compute the differences of the line's start- and endpoint.
      final double[] differences = new double[dimension];
      double fraction = 0.0;
      for (int i = 0 ; i < dimension ; ++i)
      {
         differences[i] = (float)(lastLinePoint.getComponent(i) - firstLinePoint.getComponent(i));
      }

      // Compute the fraction that represents the point where the line enters the raster.
      for (int i = 0 ; i < dimension ; ++i)
      {
         double newFraction = 0.0;
         final double component = firstLinePoint.getComponent(i);
         final double difference = differences[i];
         final double maxValue = raster.getSize(i) - 1;
         if (component < 0.0)
         {
            if (difference <= 0.0) return false;
            newFraction = component / difference;
         }
         else if (component > maxValue)
         {
            if (difference >= 0.0) return false;
            newFraction = (component - maxValue) / difference;
         }
         if (newFraction < fraction) fraction = newFraction;
      }

      // Use the fraction to compute the first point of the line that lies within the raster
      // and check if the point really lies within the raster.
      // The point may not lie within the raster if the line has no point in the raster.
      for (int i = 0 ; i < dimension ; ++i)
      {
         target[i] = Math.round((float)(firstLinePoint.getComponent(i) - fraction * differences[i]));
         if (target[i] < 0) return false;
         if (target[i] > raster.getSize(i) - 1) return false;
      }
      return true;
   }
}