Schönheitskorrekturen am Programm erbeten.

[Thomas xyz]

Hallo,

Dieses Programm funktioniert. NUr ist es nicht besonders schön geschrieben, wenn euch etwas auffällt was man ändern sollte, dann teilt es mir bitte mit.
Und bitte sagt auch kurz warum es geändert werden sollte,so kann ich was dazulernen um den Programmierstil zu verbessern.

mfg Thomas

Und nochmal danke für die Hilfe bei der erstellung des Programms.

#include <iostream> 
using namespace std;

class PRFGNode
{
public:
	unsigned int elem;
	PRFGNode *next;
	PRFGNode(int e=0, PRFGNode *n=0):elem(e),next(n){}

};

class Set
{
	public:
		Set():root(0) {}
		Set(int){Set *first; first=new Set();} //leere Menge
		Set (const Set &ver); //Kopierkonstruktor
		~Set();  //Destruktor, löscht alle Listenelemente

	    Set Vereinigung(Set &b); //UP für die Vereinigung
		Set& operator= (const Set &ver); //Zuweisung

		void insert_at_first(unsigned int x);  //einfügen in Liste
		void Ausgabe();
		bool ist_in(unsigned int y);  //Funktion die true zurückliefert, wenn y in Liste

	private:
		PRFGNode *root;

};

void Set::insert_at_first(unsigned int x)  //Einfuegen am Listenbeginn
{

	root=new PRFGNode(x,root);		

}

bool Set::ist_in(unsigned int y) //überprüfen ob y in Liste
{
	PRFGNode *hilf;
	bool test=false;  //bool-Variable test, hat Werte "true" oder "false"
	hilf=root;

	while (hilf!=0)
	{
		if((hilf->elem)==y)
			test=true;
		else
			test=false;

		hilf=hilf->next;
	}
	return test;
}

Set::Set (const Set &ver) {   // Kopierkonstruktor: initialisiere mich mit mit einer Kopie von p_l
root = 0;
PRFGNode **l = &root, //hier muss der Verweis auf den Neuen eingesetzt werden
*n; //Verweis auf den Neuen
for(PRFGNode * p = ver.root; p != 0; p = p->next){
	n = new PRFGNode (p->elem, 0);
	*l = n;
	l = &(n->next);
	}
}

Set & Set::operator= (const Set &ver) {   // Zuweisungsoperator: entsprechend dem Kopierkonstruktor

if (root != ver.root) { // keine Zuweisung an sich selbst
	delete root; // weg mit dem alten
	root = 0;
	PRFGNode **l = &root, *n; // Kopieren wie oben
	for(PRFGNode * p = ver.root; p != 0; p = p->next){
		n = new PRFGNode (p->elem, 0);
		*l = n;
		l = &(n->next);
		}
	}
	return *this; // liefert Referenz auf sich selbst
}

Set Set::Vereinigung(Set &b)
{
	Set c;
	PRFGNode *h1,*h2;
	bool test=false;
	h1=root; // Zeiger aufs 1 Element der Menge a, siehe Aufruf im void main  ( a.Vereinigung(b) )
	h2=b.root; // Zeiger aufs 1 Element der Menge b, diese wird als Parameter übergeben 

	while(h2!=0)   //zunächst werden in die neue Menge c alle Elemente von b hineinkopiert
	{
		c.insert_at_first(h2->elem);
		h2=h2->next;  //weiter in der Liste
	}

	while(h1!=0)       //hier werden die noch fehlenden Elemente aus a in c eingefügt
	{
		if(b.ist_in(h1->elem)) // es wird mit der Funktion "ist_in" geschaut, welche noch fehlen
			test=true;
		else
			test=false;

		if(test==false) //wenn das Element "h1->elem" von Menge a noch nicht in der Menge b, dann in c einfügen

			c.insert_at_first(h1->elem);  //einfügen in c

		h1=h1->next;    //mit next-Zeiger kommt man in der Liste weiter, (ähnlich i++)
	}                   //danach beginnt while-Schleife erneut

//	c.Ausgabe();
	return c;	
}

void Set::Ausgabe() //Ausgabe der Liste
{
	int anz=0,i=0;
	PRFGNode *aktuell;

	aktuell = root;

    while (aktuell != 0)
    {
		i++;
		cout << "\n" << "          Das " << i << ". Element hat den Wert " << (*aktuell).elem << "."<< "\n";
		aktuell = (*aktuell).next;
		cout << "\n";
    }

}

Set::~Set()  //Destruktor
{
	PRFGNode *voriges;
	PRFGNode *aktuell;

	aktuell=new PRFGNode;
	voriges=new PRFGNode;
	voriges=NULL;

	aktuell=root;

	while(aktuell!=0)
	{

		voriges=aktuell;
		aktuell=(*aktuell).next;
		delete voriges;
	}

}

void main()
{
	Set a, b, c;
	int d=6;

	a.insert_at_first(8);
	a.insert_at_first(9);
	a.insert_at_first(5);

	cout << "\n" <<"                     Programm zur Vereinigung zweier Mengen "<<"\n";
	cout << "\n"<<"_____________________________________________________________________________"<<"\n";

	cout << "\n" <<" Menge A" <<"\n";

	a.Ausgabe();
	cout << "\n"<<"_____________________________________________________________________________"<<"\n";

	b.insert_at_first(5);
	b.insert_at_first(3);
	b.insert_at_first(2);

	cout << "\n"<<" Menge B" <<"\n";

	b.Ausgabe();
	cout << "\n"<<"_____________________________________________________________________________"<<"\n";

	cout << "\n" <<" \x9a \bberpr\x81 \bfung ob ein bestimmtes Element in Menge A enthalten ist. " <<"\n";

	if(a.ist_in(d))  //dh wenn true zurückgegeben wird, dann ist es in der Liste
	cout<<"\n"<<"          Das Element " <<d << " ist in der Liste";
	else
	cout<<"\n"<<"          Das Element " <<d << " ist nicht enthalten."<<endl;  //wenn 6 nicht in Liste, dann Meldung... 
	cout << "\n"<<"_____________________________________________________________________________"<<"\n";

	cout << "\n"<<" Vereinigung aus Menge A und Menge B " <<"\n";
	cout << "\n";
	c=a.Vereinigung(b); //es wird die Vereinigungs-Menge ausgegeben, siehe UP: void Set::Vereinigung(Set b) 
					 //ist nicht genau das, was in der Angabe gefordert wurde, da dort die Vereinigung zurückgeliefert werden sollte, mit return- funkt. aber irgendwie nicht!!!!
    c.Ausgabe();
	cout << "\n"<<"_____________________________________________________________________________"<<"\n";

}

[Gast]

class PRFGNode 
{ 
public: 
    unsigned int elem; 
    PRFGNode *next;

private machen, um genauer kontrollieren zu können, wie diese Elemente geändert werden.

operator= eine const Referenz zurückgeben lassen, sonst ist folgendes möglich:

(a + b) = c;  // Zuweisung an a+b ?!

Aus

if((hilf->elem)==y)
        test=true;
    else
        test=false;

wird

test = (hilf->elem == <);

[rüdiger]

1. Bezeichner bitte durchgängig in einer Sprache
2.

Set (const Set &ver); //Kopierkonstruktor

solche Kommentare sind überflüssig. Jeder der C++ kann, wird wissen, dass das ein Kopiector ist
3.

Set(int){Set *first; first=new Set();} //leere Menge

äh, was soll das werden? du reservierst Speicher gibst den nicht frei und machst damit nichts
4. für PRFGNode würde ich struct nehmen, da du ja nur public-Elemente hast
5. Set::ist_in ist falsch. Mach das lieber so

...
        if(hilf->elem==y)
            return true;
        else
            return false;

6. benutz bitte einen einheitlichen Style (Set::ist_in ist zB. eingerückt und die { Klammern sind anders gesetzt, als bei Set::Set(const Set&) )
7. erzeug Variablen immer so spät wie möglich
8. int main! nicht void main!
9. im dtor reservierst du Speicher, was man eh nicht machen sollte (Exception Sicherheit). Aber du wirfst den Speicher unbenutzt und unfreigegeben weg

so mehr fällt mir jetzt nicht auf

[Bashar]

1. Namen entweder einheitlich englisch oder einheitlich deutsch (oder griechisch oder whatever)
   1a) sprechende Namen verwenden ... was soll ich mir unter PRFGNode vorstellen?
2. Warum heisst die Liste Set? Eine Menge stellt die Klasse anscheinend nicht dar
3. Kommentare sollen das warum verdeutlichen, nicht den Programmcode nochmal wiederholen (Beispiel: Set (const Set &ver); //Kopierkonstruktor)
4. umständliche Formulierung:

if((hilf->elem)==y)
            test=true;
        else
            test=false;

besser:

test = (hilf->elem == y);

5. wo ich gerade drin bin: die Funktion ist_in testet nicht, ob das angegebene Element enthalten ist, sondern ob das letzte Element in der Liste gleich dem übergebenen ist. Du solltest die Suchschleife bei Erfolg sofort abbrechen. Soviel zum Thema, das Programm funktioniere.
6. dein Destruktor hat Speicherlecks. Effektiv steht da sowas:

int *a = new int;
a = b;

und was passiert mit dem vorher allozierten int? Nichts -> Speicherleck. Warum überhaupt erst einen int allozieren?
7)

h1=root; // Zeiger aufs 1 Element der Menge a, siehe Aufruf im void main  ( a.Vereinigung(b) )

Es gibt kein void main. main muss den Rückgabetyp int haben.

Set Vereinigung(Set &b); //UP für die Vereinigung

Man nennt das Memberfunktionen, Elementfunktionen oder auch Methoden. UP bzw. Unterprogramm ist Basic-Slang.
9)

Set(int){Set *first; first=new Set();} //leere Menge

Was soll das darstellen? Weg damit. Speicherloch ausserdem.
10) Obligatorischer Hinweis: Es gibt in C++ bereits vorgefertigte Containerklassen (z.b. std::list oder std::set), es besteht also praktisch wenig Anlass, eine eigene Mengenklasse zu implementieren. Höchstens als Übung oder bei sehr speziellen Anforderungen.

Ich mach an der Stelle mal Schluss. An dem Programm ist noch einiges mehr falsch (ich seh z.B. noch ein Speicherleck im Zuweisungsoperator), vielleicht liefern das andere noch nach.

[Gast]

Zuweisungsoperator:

if (root != ver.root) { // keine Zuweisung an sich selbst

fängt keine selbstzuweisung ab. Ich würde die Adressen vergleichen:

if (&ver == this)
    return;

[itman]

Optimizer schrieb:

class PRFGNode 

Aus [cpp]if((hilf->elem)==y)
        test=true;
    else
        test=false;

wird

test = (hilf->elem == <);

Ich habe zweifle mich ob diese Konstruktion Standard-Konform ist.

[itman]

itman schrieb:

Optimizer schrieb:

class PRFGNode 

Aus [cpp]if((hilf->elem)==y)
        test=true;
    else
        test=false;

wird

test = (hilf->elem == <);

Ich habe Zweifleln ob diese Konstruktion Standard-Konform ist.

[Gast]

Vielleicht ist ja deine Tastatur standardkonform. Dann wirst du feststellen, dass ich nur um eine Taste aus Versehen nach links gerutscht bin. :p

[itman]

Optimizer schrieb:

Vielleicht ist ja deine Tastatur standardkonform. Dann wirst du feststellen, dass ich nur um eine Taste aus Versehen nach links gerutscht bin. :p

Ach, so