complex-Klasse - so ok? Stil und Coding -Tips erwünscht!

[Gumble]

Mein erster Beitrag in dem Forum - hab aber schon einiges hier gelernt
Hätte gern Tipps zu dem Quelltext hier, hoffe er ist nicht zu lang.
Templates wollte ich nicht benutzen - kenn mich da noch net aus und doubles reichen mir ja. Den Kopf hab ich abgeschnitten (History, Name usw).
Gibts noch weiter sinnvolle Funktionen die ich implementieren soll? Der Datentyp soll mal in einer Matrixklasse verwendet werden und soll natürlich einfach zu verwenden sein.
Danke schon mal für alles!

Complex.h

#ifndef COMPLEX_H
#define COMPLEX_H

#define _USE_MATH_DEFINES 
#include <iostream>
#include <cmath>

//----------------------------------------------------------------------------
// Complex
//----------------------------------------------------------------------------

class Complex
{
public:
	// constuctors & destructor
	Complex();
	explicit Complex(double re, double im = 0.0);
	Complex(const Complex& c);
	~Complex();

	// set & get functions
	inline double real() const;
	inline double& real();
	inline double imag() const;
	inline double& imag();
	double getAbs() const;
	double getArg() const ;
	double getArg_rel() const;

	// operators and member-functions	
	Complex& operator= (const Complex& c);
	Complex& operator= (const double d);
	Complex& operator+= (const Complex& c);
	Complex& operator-= (const Complex& c);
	double Complex::abs() const;
	void toPolar();	

	void print(std::ostream& outstream) const;

private:
	double re;
	double im;
	double absolut;
	double argument;
};  // End of Complex

// global functions
std::istream& operator>> (std::istream& instream, Complex& c);
std::ostream& operator<< (std::ostream& outstream, const Complex& c);

bool operator== (const Complex& c1, const Complex& c2);
bool operator!= (const Complex& c1, const Complex& c2);

const Complex operator+ (const Complex& c1, const Complex& c2);
const Complex operator- (const Complex& c1, const Complex& c2);
const Complex operator* ( const Complex& c1 , const Complex& c2 );
const Complex operator/ ( const Complex& c1 , const Complex& c2 );

#endif

Complex.cpp

#include "Complex.h"

// constuctors & destructor
Complex::Complex()	: re(0.0), im(0.0),  absolut(0.0), argument(0.0) {} 
Complex::Complex(double real, double imag): re(real), im(imag) { toPolar();}
Complex::Complex(const Complex& c)	: re(c.re), im(c.im) { toPolar();}
Complex::~Complex() { }

// set & get functions
inline double Complex::real() const	{ return re; }
inline double Complex::imag() const	{ return im; }
inline double& Complex::real()		{ return re; }
inline double& Complex::imag()		{ return im; }
double Complex::getAbs() const		{ return absolut; }
double Complex::getArg() const		{ return argument; }
double Complex::getArg_rel() const	{return (argument / M_PI);}

// operators & member-functions
Complex& Complex::operator= (const Complex& c)
{
	if (this == &c)  
		return *this;
	re			= c.re;
	im			= c.im;
	absolut		= c.absolut;		
	argument	= c.argument;

	return *this;
}

Complex& Complex::operator= (const double d)
{
	re			= d;
	im			= 0;
	absolut		= d;		
	argument	= 0;

	return *this;
}

double Complex::abs() const
{
	return sqrt(re * re + im * im );
}

Complex& Complex::operator+= (const Complex& c)
{
	re += c.re;
	im += c.im;
	toPolar();
	return *this;
}

Complex& Complex::operator-= (const Complex& c)
{
	re -= c.re;
	im -= c.im;
	toPolar();
	return *this;
}

void Complex::print(std::ostream& outstream) const
{
	outstream << re  << " + " << im << 'i';
}

void Complex::toPolar ()
{
	if (!(this->absolut = this->abs()))
	{
		this->argument = 0; 
		return;
	}
	if (this->im >= 0)
        this->argument = acos( this->re / this->absolut);
	else 
		this->argument = - acos( this->re / this->absolut);
}

//global functions
std::ostream& operator<< (std::ostream& outstream, const Complex& c)
{
	return outstream << '(' << c.real() << " + " << c.imag() << 'i' << ')';
}

std::istream& operator>> (std::istream& instream, Complex& c)
{
	return instream >> c.real() >> c.imag();
}

bool operator== (const Complex& c1, const Complex& c2)
{
	return c1.real() == c2.real() && c1.imag() == c2.imag();
}

bool operator!= (const Complex& c1, const Complex& c2)
{
	return !(c1 == c2);
}

const Complex operator+ (const Complex& c1, const Complex& c2)
{
	Complex ctemp = c1;
	ctemp += c2;
	ctemp.toPolar();

	return ctemp;
}

const Complex operator- (const Complex& c1, const Complex& c2)
{
	Complex ctemp = c1;
	ctemp -= c2;
	ctemp.toPolar();

	return ctemp;
}

const Complex operator* ( const Complex& c1 , const Complex& c2 )
{
	Complex ctemp;
	ctemp.real() =	c1.real() * c2.real() -
					c1.imag() * c2.imag();
	ctemp.imag() =	c1.imag() * c2.real() + 
					c1.real() * c2.imag();
	ctemp.toPolar();

	return ctemp;
}

const Complex operator/ ( const Complex& c1 , const Complex& c2 )
{
	Complex ctemp = c1;
	ctemp.real() =( c1.real() * c2.real() +
					c1.imag() * c2.imag() ) /
				  ( c2.real() * c2.real() +
					c2.imag() * c2.imag());
	ctemp.imag() =( c1.imag() * c2.real() -
					c1.real() * c2.imag() ) /
				  ( c2.real() * c2.real() + 
					c2.imag() * c2.imag());
	ctemp.toPolar();

	return ctemp;
}

[Gregor]

Ich halte es nicht für sehr klug, dass du die komplexe Zahl auf zwei verschiedene Weisen speicherst. Das hat 2 Nachteile:

1. Du benötigst doppelt soviel Speicherplatz.

2. Du mußt nach jeder Berechnung die Polarkoordinatendarstellung aktualisieren, was die Performance deiner Klasse deutlich verschlechtert.

Man hat dadurch zwar den Vorteil, dass man bei mehrmaligem Zugriff auf die Polarkoordinaten Zeit sparen kann, das kann man aber auch außerhalb der Klasse erreichen, wenn man es denn braucht. Zudem wird es wahrscheinlich nur selten vorkommen.

EDIT: Weitere Nachteile:

3. Du hast dadurch zum Beispiel die Methoden abs() und getAbs(). Wie soll ein Benutzer der Klasse wissen, welche Methode was macht?

4. Du hast dadurch mehr Code, die Klasse ist also komplexer. Wenn jemand eine weitere Methode zu deiner Klasse hinzufügt, ist es relativ wahrscheinlich, dass er das toPolar am Ende der Methode vergißt. Das führt dann zu nicht-konsistenten Daten.

Weitere Dinge, die ich nicht gut finde:

1. toPolar ist public, es ist aber eine Methode, die eigentlich nur Klassenintern genutzt wird. Die Methode sollte also private gemacht werden. Wenn du auf die gespeicherte Polarkoordinatendarstellung verzichten würdest, bräuchtest du natürlich eine toPolar-ähnliche Methode, die öffentlich wäre. Es sind momentan auch weitere Methoden public, die dies nicht sein sollten.

[fubar]

Hmm, wenn du schon immer alles über toPolar umrechnest, dann kannst du die Polarkoordinaten doch auch für die Berechnungen nutzen
z1*z2=r1*r2*exp(i*(phi1+phi2))
z1/z2=r1/r2*exp(i*(phi1-phi2))
und dann wieder zurück in karthesische Koordinaten...

Ansonsten schließe ich mich Gregors Meinung an.

[asdf]

ctemp.real() = ...

ich bin der Meinung das man sowas nicht nutzen sollte.

[Gumble]

Danke! Ihr habt Recht - wieder mal ein klarer Fall für "weniger ist manchmal mehr". Das getAbs hab ich auch nachträglich mit den beiden Variablen abs. und arg. eingebaut und die alte Methode glatt übersehn.

inline double& Complex::real()

soll ich lieber ein Methode "setReal" machen? Dachte per Referenz ist das schick.

[Jester]

Gumble schrieb:

Hätte gern Tipps zu dem Quelltext hier

Noch ein ganz wertvoller Tipp:
Nicht selber schreiben, sondern std::complex benutzen. Das ist fertig, performant und fehlerfrei.

Aber zum Üben ist sowas natürlich trotzdem sinnvoll.

MfG Jester

[idefix]

Ich finde du solltest Membervariablen kenntlich machen.
entweder mit _re oder m_re oder so. Damit sie von Funktionsvariablen
unterscheidbar sind. Auch wenn du sogutwie nur Membervariablen hast, können andere
schneller reinfinden.

Sorry, zu schnell gelesen. War schon dran.

inline double& real();

Hier mit machts du private Membervaribale public

//entweder
inline double real() const; 

//oder 
inline const double& real() const ;

[Bashar]

ich finde die set-Funktionen für Real- und Imaginärteil völlig überflüssig. Sowas braucht man doch nicht. Bei double gibts schließlich auch keine Funktion, mit der ich den Exponenten verändern kann.

[Gumble]

idefix schrieb:

Ich finde du solltest Membervariablen kenntlich machen.
entweder mit _re oder m_re oder so. Damit sie von Funktionsvariablen
unterscheidbar sind. Auch wenn du sogutwie nur Membervariablen hast, können andere
schneller reinfinden.

Ist das mit dem Unterstrich Konvention? Hab keine Ahnung.

idefix schrieb:

inline double& real();

Hier mit machts du private Membervaribale public

//entweder
inline double real() const; 

//oder 
inline const double& real() const ;

ich brauch ja ne Schnittstelle nach außen - denn die operator / und * und >> müssen ja die Membervariablen ändern können.
abs und arg hab ich schon rausgeschmissen, genauso wie toPolar. Die Funktionen getArg, getArg_rel und getAbs reichen völlig.

[Gregor]

idefix schrieb:

Ich finde du solltest Membervariablen kenntlich machen.
entweder mit _re oder m_re oder so. Damit sie von Funktionsvariablen
unterscheidbar sind. Auch wenn du sogutwie nur Membervariablen hast, können andere
schneller reinfinden.

Ich halte das nicht für hilfreich. Das ist IMHO Geschmackssache. Einige machen die Member nochmal extra kenntlich, andere nicht. Ich halte es für wichtiger, dass die Methoden etc. kurz und übersichtlich sind. Das ist hier wohl gegeben. Mir würde es deshalb nichts bringen, wenn die Member noch irgendein Pre- oder Postfix haben. Ich sehe auch so sofort, was ein Member ist und was nicht.

Wichtiger fände ich es, dass die Variablennamen aussagekräftiger gewählt werden. Hier ist zwar klar, wofür eine Variable "re" steht, bei einer anderen Klasse kann so eine Abkürzung aber schon nicht mehr so klar sein.

[Jester]

Aber wenn irgendwie markieren, dann nicht _ an den Anfang, das ist Vorbehalten für STL-Implementierer und Compiler-Bauer.

MfG Jester

[operator void]

Gumble schrieb:

#include <iostream>
#include <cmath>

Statt <iostream> wäre, glaube ich, <iosfwd> besser, weil <iostream> nur dafür zuständig ist, dass man auf die vier (bzw. acht) Standardstreams zugreifen kann, <iosfwd> aber istream und ostream deklariert. In der Complex.cpp musst du dann <istream> und <ostream> inkludieren, außerdem würde ich <cmath> auch dahin verschieben.

Complex(const Complex& c);
	Complex& operator= (const Complex& c);
	~Complex();

Was ist an den automatisch generierten Versionen hier auszusetzen? Die haben den Vorteil, dass sie in "normalen" Klassen immer das richtige tun und man beim Hinzufügen eines Members nichts übersehen kann.

inline double real() const;
	inline double& real();
	inline double imag() const;
	inline double& imag();

inline hilft nicht viel, wenn die Definition der Funktion in einer .cpp-Datei versteckt ist. Ja, manche Linker können auch diese Grenze teilweise überwinden, aber hier würde ich einfach schreiben:

double& real() { return re; } // Implizit inline, weil direkt in der Klasse definiert

Überhaupt muss man "inline" nur noch verwenden, um kleine Funktionen direkt in den Header schreiben zu können (d.h. keine Linkerprobleme zu bekommen), Compiler entscheiden alleine schon ganz gut, was sie inlinen.

Complex& Complex::operator= (const Complex& c)
{
	if (this == &c)  
		return *this;
    ...
}

Wozu der Selbstzuweisungstest? Der erspart nur im Ausnahmefall der Selbstzuweisung ein paar Variablenzuweisungen, ansonsten macht er die Zuweisung sogar langsamer. IMHO braucht man den Selbstzuweisungstest nur sehr selten.

Und print() kann IMHO raus, dafür gibt es op<<

[Gumble]

Statt <iostream> wäre, glaube ich, <iosfwd> besser, weil <iostream> nur dafür zuständig ist, dass man auf die vier (bzw. acht) Standardstreams zugreifen kann, <iosfwd> aber istream und ostream deklariert. In der Complex.cpp musst du dann <istream> und <ostream> inkludieren, außerdem würde ich <cmath> auch dahin verschieben.

hm, dann kann ich nimmer auf std::cout zugreifen und er wirft mir noch ein paar Fehler wegen '<<'
Dachte immer, ich steck alle includes in den Header - was ist nun usus?

Complex(const Complex& c);
	Complex& operator= (const Complex& c);
	~Complex();

Was ist an den automatisch generierten Versionen hier auszusetzen? Die haben den Vorteil, dass sie in "normalen" Klassen immer das richtige tun und man beim Hinzufügen eines Members nichts übersehen kann.

Hab ich wirklich Nachteile wenn ich das selber definier?

inline double real() const;
	inline double& real();
	inline double imag() const;
	inline double& imag();

inline hilft nicht viel, wenn die Definition der Funktion in einer .cpp-Datei versteckt ist. Ja, manche Linker können auch diese Grenze teilweise überwinden, aber hier würde ich einfach schreiben:

Ok, inline hab ich rausgeschmissen - dachte es wär gut.

double& real() { return re; } // Implizit inline, weil direkt in der Klasse definiert

Überhaupt muss man "inline" nur noch verwenden, um kleine Funktionen direkt in den Header schreiben zu können (d.h. keine Linkerprobleme zu bekommen), Compiler entscheiden alleine schon ganz gut, was sie inlinen.

soll ich nur eine Funktion (die mit der Referenz als Rückgabe) verwenden? Gibt das irgendwelche Vor- oder Nachteile?

Complex& Complex::operator= (const Complex& c)
{
	if (this == &c)  
		return *this;
    ...
}

Wozu der Selbstzuweisungstest? Der erspart nur im Ausnahmefall der Selbstzuweisung ein paar Variablenzuweisungen, ansonsten macht er die Zuweisung sogar langsamer. IMHO braucht man den Selbstzuweisungstest nur sehr selten.

Meister Meyers aus dem Effektiv C++ Buch hat das doch empfohlen, also lass ich es lieber so zur sicherheit

Und print() kann IMHO raus, dafür gibt es op<<

und weg damit

Danke nochmal für die Tips!

[operator void]

Gumble schrieb:

hm, dann kann ich nimmer auf std::cout zugreifen und er wirft mir noch ein paar Fehler wegen '<<'

Von std::cout muss deine Klasse ja auch nichts wissen Wenn du an anderer Stelle cout/cin etc. brauchst, ist <iostream> dafür der richtige Header.

Dachte immer, ich steck alle includes in den Header - was ist nun usus?

Am besten so wenig #includes wie möglich im Header. Mit Vorausdeklarationen und dem "pimpl-Idiom" kann man da sogar eine Wissenschaft für sich draus machen, damit größere Projekte schneller kompilieren und seltener neu kompiliert werden müssen, wenn sich Header ändern.

Hab ich wirklich Nachteile wenn ich das selber definier?

Du musst sie halt erstmal alle tippen. Danach gilt: Code, den man nicht tippt, muss man nicht warten Als einziger direkter Nachteil fällt mir hier nur ein, dass die automatisch generierten Versionen inline und dadurch vermutlich schneller wären (vor allem der Destruktor), wenn der Linker das nicht merkt und selber gerade biegt.

soll ich nur eine Funktion (die mit der Referenz als Rückgabe) verwenden? Gibt das irgendwelche Vor- oder Nachteile?

Nein, auf jeden Fall brauchst du zwei Funktionen: Eine, mit der man den Wert ändern kann und eine, mit der man den Wert einer "const Complex" auslesen kann. Das finde ich schon gut, wie es ist.

Meister Meyers aus dem Effektiv C++ Buch hat das doch empfohlen, also lass ich es lieber so zur sicherheit

Hm ja. Aber überleg mal, wofür der in EffC++ vermutlich gut war und warum das hier nicht zutrifft