Verbesserungsvorschläge / Korrektur

Krikus · 8. Mai 2009

Hi,

im folgenden poste ich 3 Klassen aus meinem kleinen Projekt des Praktikums an der Uni, welches ständig erweitert wird..
Ich bitte euch den Code einmal anzuschauen und Verbesserungsvorschläge zu posten, bzw. Stellen mit schlechtem Programmierstil aufzuzeigen.

C++:

class NAVI{
private:

public:

virtual void routeberechnen() = 0;
virtual void ausgeben() = 0;
virtual string get() = 0;
virtual void adv() = 0;
virtual void reset() = 0;
virtual void ins_manuel() = 0;
virtual void ins(string x, int x_ko, int y_ko) = 0;
virtual void del() = 0;
virtual bool consist_of(string x)= 0;
virtual void write_to_binary_file() = 0;
virtual void read_from_binary_file() = 0;
virtual void read_from_textfile() = 0;
virtual void write_to_textfile() =0 ;

};

C++:

class Ort 
{
	private:
		string name;
		int x_koordinate;
		int y_koordinate;    

	public:
		 Ort()
		 {
			name=""; 
			x_koordinate = 0; 
			y_koordinate = 0;
		  }

		  void set_name(string eingabe_name)
		  {
			 name=eingabe_name;
		  }
		  void set_x_koordinate(int x)
		  {
			  x_koordinate=x;
		  }
		  void set_y_koordinate(int y)
		  {
			  y_koordinate=y;
			}

		  string get_name() 
		  {
			  return name;
		  }

		  int get_x_koordinate()
		  {
			  return x_koordinate;
		  }
		  int get_y_koordinate()
		  {
			  return y_koordinate;
		  }

		
};

C++:

class deutsch : public NAVI
{

private:

	struct knoten {
	  Ort info;
	  struct knoten *next;
	};
	                
	struct knoten *pos;      // Positionszeiger
	struct knoten *pre_pos;  // Vorgänger des Positionszeigers
	struct knoten *anfang;   // Zeiger auf den Anfang  der Liste

public:

	deutsch() {
		pos = pre_pos = anfang = NULL; // Leere Liste 
		read_from_binary_file(); //vorhande Daten aus binäer Datei in die Liste laden.
	}

	~deutsch(){
		write_to_binary_file(); //vorhanden Einträge aus der Liste in Binär datei schreiben.
	}

	virtual void routeberechnen()
	{
		cout <<"Test zur Routenberechnung"<<endl;
	}

	void ausgeben() {                            // ausgeben von Anfang bis Ende
			struct knoten *p = anfang;           // p zeigt auf aktuellen Knoten
			cout <<endl<< "Orte: ";
			while (p != NULL) {                        
				cout << p->info.get_name()<<"["<<p->info.get_x_koordinate()<<"]"<<
					"["<<p->info.get_y_koordinate()<<"]"<< " ";      // Infoteil des aktuellen Knoten ausgeben
					p = p->next;                 // p auf Folgeelement setzen
			}
			cout << endl;
	}

	string get() {                    // liefere aktuelles Element
		return pos->info.get_name();
	}
	bool end() {                    // Ende erreicht?
			return (pos == NULL);                   
	}
	void adv() {                    // Positionszeiger vorrücken
			if (pos != NULL) {
					pre_pos = pos;
					pos = pos->next;
			}
	}
	void reset() {                  // Positionszeiger an den Anfang setzen
			pos = anfang;
			pre_pos = NULL;
	}

	void ins_manuel() // Manuelle Eingabe des Ortes und der Koordinaten
	{
		string x;
		int x_ko,y_ko;
        cout << endl <<"Neuen Ort hinzufuegen: ";
        cin >> x;
		cout << endl <<"X-Koordinate eingeben: ";
		cin >> x_ko;
		cout << endl <<"Y-Koordinate eingeben: ";
		cin >> y_ko;

		if (consist_of(x)==false){
			struct knoten *p = new knoten;  // erzeuge neuen knoten
			p->info.set_name(x);// Infowert wird x
			p->info.set_x_koordinate(x_ko); 
			p->info.set_y_koordinate(y_ko);
			p->next = pos;      // next wird initialisiert mit alten P.-Zeiger
								// es wird also VOR dem aktuellen Element eingefügt
			if (anfang == NULL)     // Leere Liste
					anfang = p;
			if (pre_pos != NULL)            // Positionszeiger hat einen Vorgänger
					pre_pos->next = p;
			else                            // neues Element steht am Anfang
					anfang = p;
			pos = p;              // Positionszeiger zeigt nun auf das neue Element
		}
		else{
				cout << "Der eingegebene Ort " << x << " ist bereits vorhanden"<<endl;
			}
	}
	void ins(string x, int x_ko, int y_ko) {
			struct knoten *p = new knoten;  // erzeuge neuen knoten
			p->info.set_name(x);// Infowert wird x
			p->info.set_x_koordinate(x_ko); 
			p->info.set_y_koordinate(y_ko);
			p->next = pos;      // next wird initialisiert mit alten P.-Zeiger
								// es wird also VOR dem aktuellen Element eingefügt
			if (anfang == NULL)     // Leere Liste
					anfang = p;
			if (pre_pos != NULL)            // Positionszeiger hat einen Vorgänger
					pre_pos->next = p;
			else                            // neues Element steht am Anfang
					anfang = p;
			pos = p;              // Positionszeiger zeigt nun auf das neue Element
	}


	void del(){

		string x;
		cout <<endl<<"Geben Sie den Ortsnamen ein welcher geloescht werden soll: "<<endl;
		cin >> x;
		reset(); // Reset der Zeiger pos und pre_pos  
		struct knoten *p = anfang;// p zeigt auf aktuellen Knoten
		bool status=false;
		while (p != NULL) { 

			if (x == p->info.get_name()){  //vergleicht ob Ort bereits in der Liste ist.
				if(pos!= NULL) //wenn p zeiger nicht am ende steht:  
				{  
					if (pre_pos!= NULL) // p zeiger steht nicht am anfang ( weil sonst preP auf NULL)  
					{  
						pre_pos->next =pos->next ;  
					}  
					else  
						anfang= pos->next; //anfang neu setzen  

						delete p; //löscht  
						cout <<"Ort geloescht"<<endl;
						status=true;
			 
						if(pre_pos != NULL)  
						{  
							pos=pre_pos->next; //p zeiger steht auf ehemaligen nachfolger  
						}  
						else  
							pos = anfang;  
							break;  
				}   
			}  
			adv(); // Positionszeiger weiterrücken  
			p = p->next; // p auf Folgeelement setzen  
		}  
		reset(); // Zeiger wieder resetten  

			if(status==false)
					cout << "Der von Ihnen eingegebene Ort ist nicht in der Liste!"<< endl; 		
	}


	bool consist_of(string x){

		struct knoten *p = anfang;           // p zeigt auf aktuellen Knoten
			while (p != NULL) { 
				if(p->info.get_name() == x)		//vergleicht ob Ort bereits in der Liste ist.
				{
					return true;
				}
				p = p->next;                 // p auf Folgeelement setzen
			}
		return false;
	}


void write_to_binary_file()
{       
	struct daten
	{
		char test[30];
		int x_ko;
		int y_ko;
	};
	daten p_daten;

	const char *x;
	struct knoten *p = anfang;  
	ofstream binary_file;// Zieldatei (Schreiben)
	binary_file.open("binaer.dat", ios::out|ios::binary); // zum Schreiben öffnen
	if (!binary_file) {                   // Datei kann nicht geoeffnet werden
		cerr << "binaer.dat" << " kann nicht geoeffnet werden!\n";
		getch();
	}
	while (p != NULL) {                        
					strcpy(p_daten.test, p->info.get_name().c_str());//Umwandlung String in Char
					p_daten.x_ko = p->info.get_x_koordinate();
					p_daten.y_ko = p->info.get_y_koordinate();
					binary_file.write(reinterpret_cast<char *>(&p_daten),sizeof(daten));
					p = p->next;                 // p auf Folgeelement setzen
			}

	binary_file.close();
}

void read_from_binary_file()
{
	struct daten
	{
		char test[30];
		int x_ko;
		int y_ko;
	};
	daten p_daten;

	ifstream binary_file;                        
	binary_file.open("binaer.dat", ios::in|ios::binary);  
	if (!binary_file) {                 // Datei kann nicht geoeffnet werden
		cerr << "binaer.dat" << " kann nicht geoeffnet werden!\n";
		getch();
	}
 
	while (binary_file.read(reinterpret_cast<char *>(&p_daten),sizeof(daten))) { 
		ins(p_daten.test,p_daten.x_ko,p_daten.y_ko);
	}
	binary_file.close();

}

void read_from_textfile()
{
	cout <<"Neue Ortsnamen werden nun eingelesen!"<<endl;
	int x_ko;
	int y_ko;
	string line;
	string token;
	ifstream textfile;                // Quelldatei (Lesen)          
    textfile.open("textfile.txt", ios::in); // zum Lesen öffnen
    if (!textfile) {                  // Datei kann nicht geoeffnet werden
		cerr << "textfile.txt" << " kann nicht geoeffnet werden!\n";
	}else{
		while(getline(textfile,line)){		
			stringstream iss;
			vector<string> inhalt;
			iss << line;
			while(getline(iss,token,','))
			{
				inhalt.push_back(token);
			}
			iss.clear();
        	if (consist_of(inhalt[0])==false)
			{
				// String to Int
				stringstream sstr(inhalt[1]);
				sstr >> x_ko;
				stringstream sstr2(inhalt[2]);
				sstr2 >> y_ko;
				ins(inhalt[0], x_ko, y_ko);	
			}		
		}   
	}
	textfile.close();
}


void write_to_textfile()
{
	cout <<"Aller Ortsnamen werden in die Textdatei geschrieben"<<endl;
	struct knoten *p = anfang;           // p zeigt auf aktuellen Knoten
	ofstream textfile;                 // Zieldatei (Schreiben)
	textfile.open("textfile.txt", ios::out); // zum Schreiben öffnen
    if (!textfile) {                   // Datei kann nicht geoeffnet werden
		cerr << "textfile.txt" << " kann nicht geöffnet werden!\n";
        getch();
	}
			while (p != NULL) {
				textfile << p->info.get_name()<<","<<p->info.get_x_koordinate()<<","<<p->info.get_y_koordinate()<<endl;   // Infoteil des aktuellen Knoten in Datei schreiben
				p = p->next;                 // p auf Folgeelement setzen
			}
	textfile.close();
}

};

Matthias Reitinger · 8. Mai 2009

Hallo,

was mir beim schnellen Überfliegen so aufgefallen ist:

In C++ ist es üblich und sinnvoll, die Deklaration von Klassen und deren Implementierung in zwei verschiedenen Dateien vorzunehmen (Klasse.h und Klasse.cpp).
Kommentare zu Methoden schreibt man in der Regel zur Deklaration (in die Headerdatei).
Die Benennung der Methoden ist nicht konsistent (mal Deutsch, mal Englisch, mal mit Unterstichen, mal ohne…).
Die Benennung der Klassen ist nicht konsistent (durchgehende Großschreibung, durchgehende Kleinschreibung, nur Anfangsbuchstabe groß…).
Strings sollte man besser als konstante Referenzen übergeben, um unnötige Kopiervorgänge zu vermeiden.

Die Implementierung der Methoden hab ich mir nicht näher angeschaut, darum kann ich dazu auch nichts sagen.

Grüße, Matthias

3Cyb3r · 9. Mai 2009

Zu der Variablen bennenung noch: Es ist übersichtlicher und ein guter Programmierstil die variablen am anfang so zu nennen wie sie sond, also z.B.
bein int ein i davor und bei string ein s und bei private varilen einer klasse schreibt man ein m_ davor.
Also:

C++:

class Ort
{
    private:
        string m_sName;
        int m_iXKoordinate;
        int m_iYKoordinate;  

...............................
...............................

Die bringt einem bei größeren Projekten auch relativ viel.

Matthias Reitinger · 9. Mai 2009

3Cyb3r hat gesagt.:
Zu der Variablen bennenung noch: Es ist übersichtlicher und ein guter Programmierstil die variablen am anfang so zu nennen wie sie sond, also z.B.
bein int ein i davor und bei string ein s und bei private varilen einer klasse schreibt man ein m_ davor.

Man könnte sich vortrefflich darüber streiten, ob ungarische Notation zum guten Programmierstil gehört oder nicht. Letztendlich ist es eine persönliche Präferenz, so wie jeder eine eigene Meinung hat, ob die öffnende Klammer eines Blocks in eine separate Zeile gehört oder nicht. Meine Meinung ist, dass die ungarische Notation (und damit meine ich jetzt "Systems Hungarian") ein Überbleibsel aus der Zeit ist, als einem die Entwicklungsumgebungen noch nicht mit einem Tastendruck den Typ einer Variable anzeigen konnten. Insofern empfinde ich diese Präfixe eher als störend als hilfreich.

Grüße, Matthias

deepthroat · 9. Mai 2009

Hi.

Matthias Reitinger hat gesagt.:
Man könnte sich vortrefflich darüber streiten, ob ungarische Notation zum guten Programmierstil gehört oder nicht. Letztendlich ist es eine persönliche Präferenz, so wie jeder eine eigene Meinung hat, ob die öffnende Klammer eines Blocks in eine separate Zeile gehört oder nicht. Meine Meinung ist, dass die ungarische Notation (und damit meine ich jetzt "Systems Hungarian") ein Überbleibsel aus der Zeit ist, als einem die Entwicklungsumgebungen noch nicht mit einem Tastendruck den Typ einer Variable anzeigen konnten. Insofern empfinde ich diese Präfixe eher als störend als hilfreich.

Das sehe ich genauso. Wenn man den Typ ändert muss man wieder die ganzen Variablen umbennen... Und bei der Verwendung von Templates kann man es sowieso vergessen, da man ja gar nicht weiß welcher Typ letztlich hinter der Variablen steht.

Der Präfix m_ sollte übrigens dann bei allen Membervariablen verwendet werden, nicht nur den privaten.

@Krikus: Du solltest grundlegend die Initialisierung mit einem Wert der Konstruktion per Standardkonstruktor mit nachträglicher Zuweisung vorziehen.
Nicht so:

C++:

istringstream iss;
iss << a_string;

Sondern so:

C++:

istringstream iss(a_string);

Warum sollte man etwas erstmal als leer initialisieren und danach einen Wert zuweisen, anstatt das Objekt gleich mit dem Wert zu initialisieren?!

Aus dem gleichen Grund sollte man prinzipiell Attribute einer Klasse in der Initialisierungsliste des Konstruktors initialisieren und nicht durch Zuweisung im Rumpf des Konstruktors.

Warum spendierst du der Klasse Ort nicht einen Konstruktor der das Objekt gleich mit Name, x und y Koordinate initialisiert?

Gruß

devDevil · 10. Mai 2009

(1) Warum nimmst du nicht das Klassetemplate std::list?
(2) Getter sollten const sein und const (Ref)-Wert zurückgeben
(3) Setter sollten const (Ref)-Wert übergeben bekommen bzw. solltest du das generell machen (bei Funktionen) solange du nichts an den Werten veränderst! Aber by Copy ist außer bei den primitiven Datentypen eh sehr unschön!
(4)

C++:

typedef std::pair<unsigned int, unsigned int> position_t;

(5) Initialisierungsliste
(6)

C++:

struct knoten {
      Ort info;

      struct knoten *next;

    };

sind wir bei C? ( 1. Wäre hier c-tor recht angebracht und 2. ist es fraglich ob hier next public sein sollte ... und warum C? nja weil struct bei dem next nichts zu suchen hat!)
(7) operator<< überladen anstelle der Funktion ausgeben
(8) Datenverarbeitung von UI trennen (d.h. die Ausgabe hat da nichts zu suchen!)
(9) Schreib- und Lesefuntkion sind mal richtig unschön ^^
(10) Konstruktor mit Parametern (evtl. Default-Value setzen)

Tjo hoffe das reicht erstmal ...

devDevil · 10. Mai 2009

C++:

typedef std::pair<unsigned int, unsigned int> position_t;

class Location
{
    std::string m_name;
    position_t m_position;   
 
public:
    Location(std::string const& name = std::string(), position_t const& position = position_t(0, 0))
        : m_position(position), m_name(name)
    {}
 
 public:
    void set_name(std::string const& name)
    { m_name = name; }
    void set_position(position_t const& position)
    { m_position = position; }
    
public:
    std::string const& get_name() const
    { return m_name; }
    position_t const& get_position() const 
    { return m_position; }
    
public:
    const bool operator==(std::string const& rhs) const { return m_name == rhs; }
    friend std::ostream& operator<<(std::ostream& out, Location const& rhs)
    { return out << rhs.get_name() << " (" << m_position.first << "|" << m_position.second << ")"; }
};

so könntest du z.B. die Klasse Ort aufbauen ...

Wobei ich an deiner Stelle evtl. mit Breitenkreis usw. als Koordinaten arbeiten würde

C++:

class Map
{
	std::list<Location> m_data;
	std::string m_name;
 
public:
    Map()
	{ read_from_binary_file(); }
 
    virtual ~Map()
	{ write_to_binary_file(); }
 
 public:
    friend std::ostream& operator<<(std::ostream& out, Map const& rhs)
	{ 
		out << rhs.m_name << "\n";
		std::copy(rhs.m_data.begin(), rhs.m_data.end(), std::ostream_iterator<Location>(out, "\n")); 
		return out;
	}
	
public:
	virtual void add_location(Location const& location) 
	{ m_data.push_back(location); }
	
    virtual void delete_location(std::string const& name)
	{
		std::list<Location>::iterator it(std::find(m_data.begin(), m_data.end(), name));
		if (it != m_data.end()) m_data.erase(it);
	}
 
	virtual void exists_location(std::string const& name) const
	{ return std::find(m_data.begin(), m_data.end(), name) != m_data.end(); }
 
 public:
 	void write(std::string const& filename = "binary.dat") const
	{ 
		std::ofstream file_stream(filename.c_str(), std::ios_base::binary);
		if (!file_stream) throw std::invalid_argument(filename);
		// write name of map
		write_string(file_stream, m_name);
		// write count of location on map
		const std::size_t count(m_data.size());
		file_stream.write(reinterpret_cast<const char*>(&count), sizeof(std::size_t));
		// write locations
		for (std::list<Location>::const_iterator it(m_data.begin()), end(m_data.end()); it != end; ++it)
		{
			write_string(file_stream, it->get_name());
			position_t const& position(it->get_position());
			file_stream.write(reinterpret_cast<const char*>(&position), sizeof(position_t));
		}
	}
	
	void read(std::string const& filename = "binary.dat")
	{
		std::ifstream file_stream(filename.c_str(), std::ios_base::binary);
		if (!file_stream) throw std::invalid_argument(filename);
		// read name of map
		if (!read_string(file_stream, m_name)) throw std::runtime_error("invalid file-format");
		// read cout of location on map
		std::size_t count(0);
		file_stream.read(reinterpret_cast<char*>(&count), sizeof(std::size_t));
		// read locations
		for (std::size_t i(0); i < count; ++i)
		{
			std::string name;
			if (!read_string(file_stream, name)) throw std::runtime_error("invalid file-format");
			position_t position;
			file_stream.read(reinterpret_cast<char*>(&position), sizeof(position_t));
			add_location(Location(name, position));
		}
	}
		
	
private:
	std::ostream& write_string(std::ostream& out, std::string const& string) const
	{ 
		// write length of string (+ Nulltermination)
		const std::size_t length(string.length() + 1);
		out.write(reinterpret_cast<const char*>(&length), sizeof(std::size_t));
		// write string
		out.write(string.c_str(), length);
		return out;
	}	
	
	std::istream& read_string(std::istream& in, std::string& string) const
	{
		// read length of string (+ Nulltermination)
		std::size_t length(0);
		in.read(reinterpret_cast<char*>(&length), sizeof(std::size_t));
		// read string
		char* ptr_tmp = new char[length];
		in.read(ptr_tmp, length);
		string = ptr_tmp;
		delete [] ptr_tmp;
		return in;
	}
};

so is das schreiben mal korrigiert

Und paar Stellen verbessert ....

Krikus · 10. Mai 2009

Besten Dank für die vielen Verbesserungsvorschläge.
An einige werde ich mich sofort begeben,

@devDevil
Besten Dank für deine Mühe der Verbersserungsvorschläge und des Codes.
Zu einigen Punkten habe ich allerdings noch Fragen.

1.
-> Weil wir ADT einige Wochen vorher in der Vorlesungen hatten, sollen wir unsere eigene Klasse Liste verwenden.
7.
-> hatten wir noch nicht in der Vorlesung. Aber wenn du Zeit und Lust hast, kannst du da ein Beispiel dazu posten.
8.
-> Du meinst sicher GUI und nich UI oder? Weil UI kenne ich nicht.
Siehe ich das richtig das z.B die Eingabe von den Variablen nich in der Methode geschen soll, sondern in der Main-Funktion, und die Variablen dann als Parameter übergeben werden sollen?

devDevil · 10. Mai 2009

1) Dann mach dir halt daraus nen anständiges Klassentemplate ...
7) Ist oben schon drin. Dadurch kannst du dann z.B. einfach schreiben

C++:

std::ofstream file_stream(/* ... */);
Location loc("insel", position_t(10, 10));
file_stream << loc << std::endl; // einmal in datei
std::cout << loc << std::endl; // einmal in console

8) (General) User Interface

... ist mir gleich

Ehm ja genau das meine ich

... ob jetzt in der main oder in extra funktionen ist dabei relativ egal

Aber die Eingabe hat ja nichts mit der grundlegenden Datenstruktur von Lokation zu tun, oder?

Verbesserungsvorschläge / Korrektur

Krikus

Mitglied

Matthias Reitinger

ɐɯıǝɹ

3Cyb3r

Erfahrenes Mitglied

Matthias Reitinger

ɐɯıǝɹ

deepthroat

Erfahrenes Mitglied

devDevil

Erfahrenes Mitglied

devDevil

Erfahrenes Mitglied

Krikus

Mitglied

devDevil

Erfahrenes Mitglied

Neue Beiträge