OOP Frage zu Programmier-Entscheidungen

[Juggl3r]

Hallo,

ich habe einige Fragen, zu gewissen Programmier-Entscheidungen. Die ganzen Fragen möchte ich gerne an Hand eines Beispiels stellen. (eine Klasse zum Sortieren von Arrays)

Ich poste euch hier mal meinen Code:

Sortierverfahren.java

package de.juggl3r.utils;

public class Sortierverfahren {
	private int[] daten;
	
	Sortierverfahren(int[] daten) {
		// Daten Kopieren, damit übergebene nicht verändert werden
		this.daten = new int[daten.length];
		for(int i = 0; i < daten.length; ++i)
			this.daten[i] = daten[i];
	}
	
	public int[] getDaten() {
		int[] returnDaten = new int[daten.length];
		for(int i = 0; i < daten.length; ++i)
			returnDaten[i] = daten[i];
		return returnDaten;
	}
	
	public void bubblesort() {
		int temp;
		for(int i = daten.length -1; i > 0; --i) {
			for(int k = 0; k < i; ++k) {
				if(daten[k] > daten[k+1]) {
					temp = daten[k];
					daten[k] = daten[k+1];
					daten[k+1] = temp;
				}
			}
		}
	}
	
	public void selectionsort() {
		int temp;
		for(int i = 0; i < daten.length -1; ++i) {
			int tempForIndex = i;
			for(int k = i+1; k < daten.length; ++k) {
				if(daten[k] < daten[tempForIndex])
					tempForIndex = k;
			}
			temp = daten[i];
			daten[i] = daten[tempForIndex];
			daten[tempForIndex] = temp;
		}
	}
}

Zum einfachen testen habe ich nun folgende Klasse geschrieben:

package de.juggl3r.utils;
import java.util.Random;

public class Testprogramm {

	private static final int ANZAHL_ZUFALLSZAHLEN = 100;
	private static final int ZUFALLSZAHLEN_MIN = 0;
	private static final int ZUFALLSZAHLEN_MAX = 100;
	private static int[] daten;
	
	/**
	 * @param args
	 */
	public static void main(String[] args) {
		zufallszahlen_generieren();
		Sortierverfahren instanz = new Sortierverfahren(daten);
		zufallszahlen_anzeigen();
		instanz.selectionsort();
		daten = instanz.getDaten();
		zufallszahlen_anzeigen();
	}

	
	private static void zufallszahlen_generieren() {
		daten = new int[ANZAHL_ZUFALLSZAHLEN];
		Random rnd = new Random();
		for(int i = 0; i < ANZAHL_ZUFALLSZAHLEN; ++i)
			daten[i] = ZUFALLSZAHLEN_MIN + rnd.nextInt(ZUFALLSZAHLEN_MAX-ZUFALLSZAHLEN_MIN+1);
	}
	
	private static void zufallszahlen_anzeigen() {
		for(int i = 0; i < daten.length; ++i)
			System.out.println("Zufallszahl " + i + " : " + daten[i]);
	}
}

So nun meine Frage:
1) Kann ich die Variablendeklerationen in Testprogramm so lassen? Ich lese in einem Buch (Clean Code), dass am Besten Variablen, welche oft in der Klasse vorkommen, als Variablen für die ganze Klasse (bzw. für die ganze Instanz) dekleriert werden sollen. Dadurch erhält man eine übersichtliche Parameterliste und wird nicht abgelenkt. In anderen Büchern (Programmierbücher von Java oder Eclipse) steht, dass man den Geltungsbereich von Variablen so klein wie möglich halten sollte, also z.b. die Variable Daten in main erstellen soll und dann per parameter an die Funktionen übergeben soll. Was ist den nun wirklich guter Programmierstil und was würdet ihr mir empfehlen?

2) Beim Konstruktoraufruf von Sortierverfahren kopiere ich das ganze Array. WÜrde ich es nicht kopieren, so würde mit dem Original-Array gearbeitet und d.h. die Daten würden verändert (was vll. viele Benutzer der Klasse irritieren könnte). Andererseits benötige ich allerdings, wenn ich eine tiefe Kopie erstelle, eben viel Länger Zeit, wenn das Array groß wird. Außerdem eine zusätliche Getter Funktion + einer tiefen Kopie im Getter, da ja sonst der User meine Daten ändern könnte (also zuerst aufruf von sortieren, dann holt er sich daten über getter fkt, dann ändert er bei sich daten und damit auch in der Klasse Sortierverfahren und dann würde er beim erneuten Getter Aufruf fälschliche Daten erhalten). D.h. "nur" damit die Klasse benutzerfreundlicher wird, nehme ich solche Einbühsen entgegen - ist das gerechtfertigt oder würdet ihr das anders designen?

3) Normalerweise sollte man das ganze ja per Unit-Test testen und nicht so "billig" über eine main-Fkt. Allerdings weiß ich nicht wirklich, wie ich das machen soll. Ich habe ja über den Konstruktoraufruf die Übergabe der daten, wie mache ich das mit Unit-Test? Einfach eine Instanz erstellen und dann über AssertEquals()? Kann ich dadurch dann auch mehrere Testfälle in eine TestCase-Klasse packen?

4) Gibts sonst noch anmerkungen und verbesserungsvorschläge zu meinem Stil (und dem Code?)

 

[Volvagia]

1) Das kommt wohl auf deinem Stil an. Mich persönlich würden Variablen in der Klasse stören, die nur in einer Methode verwendet werden. Außerdem würde ich meistens warscheinlich das resetten der Werte vergessen. Aber ich denke, du solltest es so machen, wie du am Besten zurecht kommst. Der beste Stil nützt dir nichts, wenn du dir beim verwenden sehr schwer tust.

2) Würde ich auf jedem Fall. Stell dir vor das Source-Array würde von einem anderen Thread wärend dem Sortiervorgang verändert werden. Wenn es dabei zu einem Fehler kommt fällt es sicher auf dich zurück.

3) Damit kenn ich mich nicht aus, mache es immer über die Main.

4) Wenn du die Klasse für andere schreibst, solltest du vielleicht einen kurzen JavaDoc-Comment reinschreiben. Ich würde die Methode bubblesort() private machen, den Getter syncronizieren, sie direkt dort aufrufen, und wenn er fertig ist eine boolean setzen damit ers kein 2. mal macht, oder direkt einen Thread daraus machen, gleich im Konstruktor starten (final!), und im Getter den Thread joinen. Ausdem würde ich noch paralell einen Konstruktor für Collections anbieten. Und Methoden schreibt man klein und CamelCase, und Klassen groß und CC. Ist nicht überall der Fall.
PS: System.arraycopy.

 

[VfL_Freak]

Moin,

nebenbei noch ein Tipp um Deinen Code hier lesbar zu gestalten :
ein schließendes Tag benötigt noch eine Slash - also [/JAVA]

Gruß
Klaus

 

[Landei]

1) Instanzvariablen sollten nur die Variablen sein, die wirklich "Bestandteil" des Objekts sind. "Wiederverwendung" von Variablen bringt normalerweise keine messbare Performancesteigerungen, und führt nur zu Konfusion, insbesonder wenn (wie du selbst schon schreibst) ein "resetten" nötig ist. Für Klassenvariablen sieht es ähnlich aus, mit der Ausnahme, dass man da eventuell Objekte, von denen man nur eine Instanz (und diese ohne resetten) als eine Art "Dienst" braucht, als final static private definieren kann. Typische Beispiele wären Logger-, Random-, Calendar- und Formatter-Objekte (wobei hier Vorsicht geboten ist, wenn man mit Multithreading arbeitet. Calendar und Formatter sind nicht threadsafe).

2) Ich würde das Array auf jeden Fall kopieren. Eventuell kannst du noch eine zusätzliche Methode anbieten, die das ohne Kopieren ("in place") macht, aber dann solltest du sie entsprechend benennen (diesen Stil findet man oft in 3D-Mathe-Bibliotheken, wo z.B. eine normale Matrixmultiplikation und aus Performancegründen auch eine "überschreibende" Version angeboten wird).

3) Vielleicht solltest du dein Design überdenken. Der Mehrwert, den du durch dein Objekt hast, ist hier gleich null. Insbesondere kannst du ein Objekt nur für einen Sortiervorgang verwenden, was doof ist.

Im Prinzip ähnelt deine Klasse den Klassen Math und Collections: Du bekommst etwas, berechnest etwas und lieferst etwas zurück - aber dazu braucht deine Klasse keine "Identität". Du bietetst eher eine Sammlung von "Diensten" an, die du genausogut als statische Methoden anbieten könntest.

Andererseits sind alle deine Methoden nur Varianten desselben Dienstes "Array Sortieren". Also könnte man deine Klasse auch in mehrere "Versionen" umorganisieren:

public interface ArraySorter {
   public final static ArraySorter BUBBLE_SORT = new BubbleSort();
   public final static ArraySorter INSERTION_SORT = new InsertionSort();
   int[] sort(int[] array);
}

public BubbleSort implements ArraySorter {
   public int[] sort(int[] array) {
      ...
   }
}

public SelectionSort implements ArraySorter {
   public int[] sort(int[] array) {
      ...
   }
}

Der Vorteil wäre dann, dass deine Sortierverfahren "austauschbar" und auch leicht erweiterbar würden. Du könntest z.B. eine Methode schreiben, die einen ArraySorter als Argument übernimmt, und diesen intern verwendet. Beim Aufruf dieser Methode könntest du dir den am besten geeigneten Sortierer "aussuchen" (ähnlich wie bei Collections.sort den Comparator). Wenn du einen schnelleren Algorithmus geschrieben hast (etwa Quicksort), lässt sich dieser dann viel einfacher "einschleusen", als wenn du an hunderttausend Stellen bubbleSort()-Aufrufe stehen hast.

3) Würde sich mit der Architektur unter 2. auch vereinfachen. Du must zwar für jeden Algorithmus eine Instanz erstellen, kannst diese aber mehrfach verwenden. Außerdem kannst du bis auf die Konstruktion für jeden Algorithmus denselben Code verwenden:

@Test
public testBubbleSort() {
    sortTests(ArraySorter.BUBBLE_SORT);
}

@Test
public testSelectionSort() {
    sortTests(ArraySorter.SELECTION_SORT);
}

private sortTests(ArraySorter sorter){
   //hier die eigentlichen Tests
}

4) Nö, geht so.

 

[Marcinek]

Hallo,

zu 1) Die wahl der Klassenvariablen / Instanzvariablen / Lokalen variablen wird NICHT aufgrund ihres Vorkommens in Programmen / Klassen gewählt. Wenn du ein Haus hast, dann würden Türen, Fenster, Mieter als Instanzvariablen auftauchen, weil sie das Objekt beschreiben. Klassenvariablen würden Anzahl aller Häuserinstanzen sein.

zu 4) Schon mall nach JUnit gegoogelt. Da steht dann, wie man korrekte Units tests macht.

 

[Juggl3r]

Ok, großen Dank schonmal für eure wirklich sehr guten Antworten!

Eine Frage hätte ich allerdings noch, was ist, wenn ich folgendes Design mache:
Ein allgemeines Interface für Sortierverfahren. (dort definiere ich die Methode execute)

Dann erstelle ich für jedes Sortierverfahren eine Klasse und implementiere das Interface. Anschließend übergebe ich immer über den Konstruktor die Daten und über die Methode execute() wird das jeweilige Sortierverfahren angewandt. Die Methode execute() gibt direkt eine Kopie des sortierten Arrays zurück. Dadurch könnte ich in einer Hashmap jeweils einen String und eine Implementierung des Interfaces Sortierverfahren abspeichern. D.h. falls jemand "bubblesort" eingibt, würde dann der jeweils entsprechende Eintrag aus der Hashmap aufgerufen werden (und das wäre eben der Algorithmus). Wäre diese Implementierung eleganter?

*) Den Unterschied zwischen Klassenvariablen und Instanzvariablen kenne ich schon, allerdings war die Frage eher auf lokale und Instanzvariablen bezogen. D.h.: in der Funktion zum Zufallszahlen generieren, würdet ihr dort alle 3 final Variablen hineinziehen, da sie sonst nie benötigt werden? Und Daten würde ich als Instanzvariable lassen?

*) Was genau meinst du mit CamelCase? Meinst du, dass ich bei Funktionsnamen so wie zufallszahlen_anzeigen lieber zufallszahlenAnzeigen verwenden sollte?

So danke nochmal für die wirklich ausführlichen antworten!

 

[Juggl3r]

Sorry für Doppelpost, bin hier aber (noch!) nicht registriert, deshalb konnte ich nicht editieren.

Wäre nicht eine noch bessere Implementierung, wenn anstelle eines Interfaces eine abstrakte Klasse verwendet wird? Dann müsste ich nicht in jedem Konstruktor den Code wiederholen, sondern könnte ihn ein mal schreiben und danach über super(daten[]) weitergeben?

 

[Marcinek]

Ja das ist mit sicherheit eine deutliche verbesserung.

 

[ARadauer]

Was genau meinst du mit CamelCase? Meinst du, dass ich bei Funktionsnamen so wie zufallszahlen_anzeigen lieber zufallszahlenAnzeigen verwenden sollte?

ja

 

[Marco13]

Die Frage, ob man den Array kopiert oder nicht ist ein bißchen heikel. Natürlich gibt es formale Aspekte, die für eine Kopie sprechen. Aber rein praktisch: Du hast selbst schon angedeutet, dass der Array ja sehr groß sein könnte, und einem dann der speicher platzt.

Ich finde, ein ganz entscheidender Punkt ist an diesen Stellen die "Emulierbarkeit" - die Möglichkeit, eine Funktion mit einer anderen Funktion "nachbauen" zu können. Wenn du deine Methode schreibst als

int[] erstelleSortierteKopie(int array[])
{
    int kopie[] = array.clone();
    sortiereInPlace(kopie);
    return kopie;
}

oder wie du es gemacht hast, mit diesem "getDaten" (was IMHO aus den oben schon von anderen genannten Gründen nicht so schön ist), dann hat der Benutzer keine Möglichkeit, dort einen Array "in-Place" (ohne eine Kopie zu erstellen) zu sortieren.

Wie man im Code schon sieht, wäre eine Methode, die in-Place sortiert aber durchaus geeignet, um die kopierende Methode zu emulieren: Man wickelt das Erstellen der Kopie einfach um die in-Place-Methode drumrum...

Ich würde dir zu einer in-Place lösung raten. Der Punkt, dass das "den Benutzer verwirren" könnte, kann leicht erledigt werden:

/** 
  * This method sorts an array <strong>in-place</strong>,
  * that means that really the given array is sorted...
  ...

Trotzdem ist die Frage nach der Methodensignatur noch interessant. An einigen Stellen (nicht so sehr in diesem Fall, aber in ähnlichen Fällen) kann es auch sinnvoll sein, "das Ziel" mit zu übergeben:

/**
 * Does XXX with the input array, writes the
 * result into the output array, and returns
 * the output array. If the output array is
 * 'null' then a new array is created, filled
 *  and returned
 */
int[] computeSomething(int input[], int output[]) { ... }

Theoretisch würde das auch für das Sortieren passen:

int[] sort(int input[], int output[]) 
{
    if (output == null) output = input.clone();
    else if (input!=output) copy(input, output);
    return sortInPlace(output);
}

oder so, aber das muss man sich dann genauer überlegen....

 

[Landei]

Wäre nicht eine noch bessere Implementierung, wenn anstelle eines Interfaces eine abstrakte Klasse verwendet wird? Dann müsste ich nicht in jedem Konstruktor den Code wiederholen, sondern könnte ihn ein mal schreiben und danach über super(daten[]) weitergeben?

Ja das ist mit sicherheit eine deutliche verbesserung.

Nein, ist es nicht. Wenn ich eine ArraySorter-Instanz habe, sollte ich damit beliebig oft sortieren können. Wenn ich über den Konstruktor gehe, um das Array zu übergeben, kann ich die Instanz nach dem Sortieren wegwerfen, für das nächste Array brauche ich dann jedesmal eine neue. Deshalb sollte das Array direkt der Sortiermethode übergeben werden. Und wenn im Konstruktor nichts übergeben wird, brauche ich auch keine abstrakte Klasse.

Um zu verdeutlichen, warum ein Interface besser ist als eine abstrakte Klasse: Nehmen wir an, du findest in einer Bibliothek schon einen fertigen Quicksort. Dann kannst du ihn ganz einfach so erweitern, dass er das Interface implementiert:

public interface ArraySorter {
  public int[] sort(int[] array);
}

//eine fremde Implementierung, für die man keinen Sourcecode hat
public class QuickSort {
  public int[] quickSort(int[] array){...}
}

public MyQuickSort extends QuickSort implements ArraySorter {
  public int[] sort(int[] array) { return quickSort(array); }
}

Mit einer abstrakten Klasse als Basis für deine Sortierer ginge das nicht, da käme nur Komposition in Frage. Bei einer einzigen Methode ist das zwar noch ein relativ überschaubarer Aufwand, aber spätestens wenn es zehn, fünfzehn Methoden werden, ist Vererbung wesentlich bequemer.

 

[Juggl3r]

Danke, für die vielen wirklich hiflreichen Tipps!

Ich habe nun alles ein bisschen umgeschrieben, wäre nett, wenn jemand kurz drüber schauen könnte und mir (eventuelle) Fehler sagen könnte. Ich will nicht, dass ich mir von Anfang an Fehler angewöhne, deshalb bin ich da für Tipps und Anregungen immer offen.

Hier das Eclipse Projekt:
Sortierverfahren.zip ... at ul.to - Free File Hosting, Free Image Hosting, Free Music Hosting, Free Video Hosting, ...

 

[Volvagia]

Sortierverfahren benötigen ja eigendlich keine Klasseninstanzen und müssen bloß einmal/Source ausgeführt werden.. Wäre es da nicht einfacher und übersichtlicher, alles in eine Klasse mit privaten Konstruktor zu stecken und die Methoden static zu machen? Ähnlich wie die Methoden in Math.

 

[Juggl3r]

meine eigentliche Idee war, dass ein Benutzer ein Sortierverfahren eingibt, bspw.: "bubblesort" und daraufhin die methode von Bubblesort aufgerufen wird.
Das ganze wollte ich so realisieren, dass ich eine Hashmap erstelle, mit Strings und Instanzen von Arraysorter. D.h. ich füge nur noch in die Hashmap ein: map.put("bubblesort", new Bubblesort());

Jz muss ich nur noch über map.get(eingabe).executeInPlace(daten) den jeweiligen Aufruf machen.
Würde ich jetzt aber alles in eine Klasse schieben, würde 1) diese Klasse um einiges größer (im Buch Clean Code steht, dass Klassen klein gehalten werden sollten) und 2) würde ich dann obigen Code nicht verwenden können, sondern müsste über if-Anweisungen (bzw. später in Java7 einmal mit switch()) die Eingabe des Users überprüfen und dann jeweils einen anderen Methodennamen aufrufen. Das wäre doch umständlicher oder?
Stimmen diese überlegungen und gibts sonst noch etwas am Code auszusetzen?

Wenn ich den allgemeinen aufbau so lassen würde, aber nur die Methoden auf static setze, würde mir das aber auch nicht viel bringen, da ich dann den Ansatz mit hashmap wieder vergessen kann.

 

[Marcinek]

Du kannst via Reflection auch auf statische Methoden zugreifen.

Statt den Instanzen könntest du auch referenzen auf die Methoden reinlegen.

Aber generell spricht nix dagegen es so zu machen, wie du es machst.

 

[Juggl3r]

Danke,

da ich noch nicht weiß, was reflections sind, belasse ich das ganze mal so
Danke nochmals an alle!

 

[Marco13]

Du könntest das mit den statischen Methoden und der HashMap auch so machen

class SortMethods {
    public static void bubbleSort(int array[]) { ... }
    public static void quickSort(int array[]) { ... }
}

interface Sorter { void sort(int array[]); }

class BubbleSorter implements Sorter {
    public void sort(int array[]) { 
        SortMethods.bubbleSort(array);
    }
}
class QuickSorter implements Sorter {
    public void sort(int array[]) { 
        SortMethods.quickSort(array);
    }
}


Map<String, Sorter> map = new HashMap<String, Sorter>();
map.put("bubbleSort", new BubbleSorter());
map.put("quickSort", new QuickSorter());

Um dieses Thema ging es auch schon in dem Thread http://www.java-forum.org/allgemein...en-statische-utility-methoden.html#post491847 . Dort wurde eine ähnliche angedeutete Lösung zwar als "Unfug" bezeichnet, aber ... ohne einen Grund zu nennen, und vermutlich nur auf genau den dortigen Zusammenahng bezogen, wo es eher etwas seltsam aussah als hier.

Ich finde das eigentlich ganz gut: Man hat ein sauberes interface, und verscheidene Implementierungen, und dass diese Implementierungen (meistens, aber eben nicht notwendigerweise) nur aus dem Aufruf einer statischen Methode bestehen, stört ja nicht - die Implementierung geht ja niemanden was an.


EDIT: Ja, ich sehe gerade, Landei hat zumindest sowas ähnliches auch oben angedeutet. Das macht doch Mut