[CODECHECK] Erstes Programm

[Rolli]

Hallo zusammen,

ich habe eine Klasse geschrieben, mit der Operationen auf Brüche durchgeführt werden können. Funktioniert soweit auch, allerdings sind im Code sicher viele Designfehler. Ich würde euch bitten mal drüber zu schauen und mir Anregungen zu geben, wie man etwas besser machen können. Z.b. Exceptions zu benutzen, aber auch wie intern mit den Objekten umgegangen wird, denke da kann man noch viel besser machen!

public class Bruch {

	private int zaehler;
	private int nenner;
	
	public Bruch(int a, int b) {
		
		this.setZaehler(a);
		if (pruefeNenner(b) == false) {
			System.out.println("Nenner darf nicht <= 0 sein...!");
			System.exit(0);
		} else {
			this.setNenner(b);
		}

	}
	
	public int getZaehler() {
		return zaehler;
	}
	
	public int getNenner() {
		return nenner;
	}
	
	public void setZaehler(int a) {
		zaehler = a;
	}
	
	public void setNenner(int b) {
		if (pruefeNenner(b) == false) {
			System.out.println("Nenner darf nicht <= 0 sein...!");
			System.exit(0);
		} else {
			nenner = b;
		}
	}
	
	// addiert zwei Brueche und liefert dann das Ergebnis zurueck
	public Bruch addieren(Bruch b) {
		
		Bruch[] brueche = new Bruch[] { this.normieren(), b.normieren() };
		int nenner = brueche[0].getNenner() * brueche[1].getNenner();
		
		// auf Hauptnenner bringen
		for (Bruch temp : brueche) {
			temp.setZaehler( ( nenner / temp.getNenner() ) * temp.getZaehler() );
			}
		
		Bruch ret = new Bruch (brueche[0].getZaehler() + brueche[1].getZaehler(), nenner);
		return ret.normieren();
	}
	
	// subtrahiert zwei Brueche und liefert dann das Ergebnis zurueck
	public Bruch subtrahiere(Bruch b) {
		
		Bruch[] brueche = new Bruch[] { this.normieren(), b.normieren() };
		int nenner = brueche[0].getNenner() * brueche[1].getNenner();
		
		// auf Hauptnenner bringen
		for (Bruch temp : brueche) {
			temp.setZaehler( ( nenner / temp.getNenner() ) * temp.getZaehler() );
			}
		
		Bruch ret = new Bruch (brueche[0].getZaehler() - brueche[1].getZaehler(), nenner);
		return ret.normieren();
	}
	
	// multipliziert zwei Brueche und liefert dann das Ergebnis zurueck;
	public Bruch multiplizieren(Bruch b) {
		Bruch[] brueche = new Bruch[] { this.normieren(), b.normieren() };
		Bruch ret = new Bruch(brueche[0].getZaehler() * brueche[1].getZaehler(),
							   brueche[0].getNenner() * brueche[1].getNenner());
		
		return ret.normieren();
		
	}
	
	// dividiert zwei Brueche und liefert dann das Ergebnis zurueck;
	public Bruch dividieren(Bruch b) {
		Bruch[] brueche = new Bruch[] { this.normieren(), b.normieren() };
		Bruch ret = new Bruch(brueche[0].getZaehler() * brueche[1].getNenner(),
							   brueche[0].getNenner() * brueche[1].getZaehler());
		
		return ret.normieren();
		
	}
	
	// Wenn Nenner nicht in Ordnung ist, wird false zurueckgegeben
	private static boolean pruefeNenner(int nenner) {
		if (nenner <= 0) {
			return false;
		} else {
			return true;
		}
	}
	
	// nimmt einen Bruch entgegen und normiert diesen
	private Bruch normieren() {
		
		Bruch temp = new Bruch(this.getZaehler(), this.getNenner());
		int ggt = ggt(Math.abs(temp.getZaehler()), temp.getNenner());
		temp.setZaehler(temp.getZaehler() / ggt);
		temp.setNenner(temp.getNenner() / ggt);
		return temp;
						
	}
	
	private static int ggt(int x, int y) {
		while (y != 0) {
			int temp = x % y;
			x = y;
			y = temp;
		}
		
		return x;
	}
}

Main

public class Main {

	public static void main(String[] args) {
		Bruch c;
		Bruch a = new Bruch(-3,5);
		Bruch b = new Bruch(2,1);
		c = a.addieren(b);
		System.out.println(c.getZaehler()+"/"+c.getNenner());
		

	}

}

Wenn ich bei manchen Methodenaufrufen das this weglasse, fügt der Compiler das dann automatisch wieder an?

Dank euch schon mal

Viele Grüße

 

[gman]

[c] System.exit(0);[/c] würde ich aus dem Konstruktor und "setNenner" rausschmeissen und durch
[c]throw new IllegalArgumentException("Nenner darf nicht <= 0 sein!");[/c] ersetzen. Den Rest guck
ich mir in Ruhe nochmal an.

 

[gman]

Hi, ich hab nochmal n bischen was zum Konstruktor. Mein Netbeans hat da was interessantes
angemeckert ;-)

Du solltest Zähler und Nenner im Konstruktor nicht über die Setter-Methoden besetzen, denn
diese könnten ja von einer Unterklasse überschrieben werden und man kann dann ja nicht sicher
wissen ob der Konstruktor noch so funktioniert wie gewünscht. Also eher sowas:

    public Bruch(int a, int b) {
        this.zaehler = a;
        if(pruefeNenner(b)) {
            this.nenner = b;
        } else {
            throw new IllegalArgumentException("Nenner darf nicht <= 0 sein!");
        }
    }

Da du das "normieren" fast überall benutzt könntest du es auch gleich im Konstruktor erledigen.
Dann musst du die Methode auch so umschreiben, das die Parameter vom Konstruktor verwendet
werden und kein neues Objekt von Bruch erzeugt wird (konstruieren im Konstruktor ist mitteldämlich).

 

[fjord]

Für ein erstes Programm sieht das doch schonmal gar nicht schlecht aus.

Du prüfst sowohl im Konstruktor als auch in setNenner, ob der Nenner ein gültiger Wert ist. Doppelten Code am Besten vermeiden. Du kannst im Konstruktor einfach setNenner nutzen.

Beim Prüfen würde ich, wie du auch schon gesagt hast, Exceptions benutzen. Für ein ungültiges Argument bietet sich da häufig die IllegalArgumentException an. In dem Zusammenhang stellt sich mir auch die Frage, warum der Nenner nicht negativ sein darf?

Dann würde ich noch vorschlagen die Klasse immutable zu machen. Das bedeutet, nachdem man ein Objekt erstellt hat, kann man es nicht mehr ändern. Dafür müsstest du lediglich setZaehler und setNenner private machen. Der Vorteil an immutable Klassen ist, dass sie automatisch threadsicher sind.

/edit:
Zu dem Punkt von gman: man könnte die Klasse auch final machen, damit sie nicht überschrieben werden kann.

/edit2:
ganz vergessen: this kannst du weglassen. Du brauchst es zum Beispiel, wenn du innerhalb einer Funktion eine Variable mit demselben Bezeichner wie eine Klassenvariable hast.

 

[gman]

@fjord: Nun, mit privaten Settern hätte sich das "Problem" ja auch gelöst. Und diese Klasse ist eigentlich prädestiniert
dafür Immutable zu sein, hast recht.

 

[Rolli]

Hi,

dank euch für die Anregungen und das drüber schauen. Eine Frage zu this habe ich aber noch

Bruch[] brueche = new Bruch[] { this.normieren(), b.normieren() };

in Zeile 43. Ist das this hier Pflicht oder nicht? Wenn ich es weglasse, wird es dann automatisch vom Complier eingefügt?

Und sollte man intern mit dem Objekt arbeiten oder besser intern mit neuen Objekten arbeiten und wenn das Ergebnis dann feststeht es entweder dem eigentlichen Objekt übergeben oder ein neues returnen? Wenn ich intern mit dem aufrufenden Objekt arbeite verändere ich ja die Dinge direkt an dem Objekt, was evtl. nicht gewünscht ist durch den Aufrufer, weil der danach noch mit dem alten Zustand weiterarbeiten möchte?!

 

[XHelp]

Lass es doch weg und probiere es aus Aber ja, du kannst es weglassen.
Ob du das Objekt veränderst oder liegt, liegt ganz bei dir. Aber wenn, dann überall. Wenn du eine immutable Klasse machst (wie String) dann darfst du niergenswo das Objekt verändern, ansonsten solltest du nicht irgendwo doch ein neues Objekt zurückgeben.

Was mir noch einfällt ist die unnötige Verwendung von Arrays. Wozu machst du zuerst alle Werte in ein Array und arbeitest damit? Du hast doch eh nur 2 Werte bei den Rechenoperationen.