FileStream

[GoOber]

Ich Programmiere seit kurzem C++ und bin grad dabei ein bisschen mit den
filestreams herumzuspielen^^ .... da hab ich mal ein Programm geschrieben und wollte wissen ob ihr vorschläge hättet^^ ..... mal abgesehen davon dass alles noch unkommeniert ist^^ ... das kommt noch....

#include <iostream>
#include <fstream>
#include <string>

using namespace std;

struct daten
{
	int id;
	int zNr;
	int tarif;
	string name;
	string kB;
}d;

void fehlerCheck();
void idPrüfen();
void idErstellen();
void datenAuslesen();

const string F_Kunde="kunde.txt";

int main()
{
	string option;

	do
	{
		system("cls");
		system("color 0A");

		cout << "Um das Programm zu beenden geben sie die <0> als ID ein." << endl << endl
			 << "Geben sie ihre Identifikationsnummer ein." << endl
			 << "ID: ";
		cin >> d.id;
		fehlerCheck();

		if(d.id==0)
		{		
			exit(0);
		}

		idPrüfen();

		if(d.id!=d.zNr)
		{
			option="";
			do
			{
				system("cls");
				cout << "Die eingegebene ID existiert nicht in unserer Datenbank." << endl
					 << "Moechten sie diese ID erstellen?" << endl
					 << "(ja/nein): ";
				cin >> option;

				if(option=="ja")
				{
					idErstellen();
				}
			}
			while(option!="ja" && option!="nein");
		}
	}
	while(option=="nein");

	datenAuslesen();
}

void fehlerCheck()
{
	cin.clear();
	cin.ignore(cin.rdbuf()->in_avail());
}

void idPrüfen()
{
	ifstream sin;

	sin.open(F_Kunde.c_str(), ios::in | ios::beg);
	if(sin.good())
	{
		do
		{
			sin >> d.zNr >> d.name >> d.tarif >> d.kB;
		}
		while(!sin.eof() && d.id!=d.zNr);
	}
	sin.close();
}

void idErstellen()
{
	ofstream sout;

	system("cls");

	cout << "Bitte geben sie ihre Daten sorgfaeltig ein." << endl << endl;
	system("pause");

	system("cls");

	cout << "Geben sie ihren Namen ein: ";
	cin >> d.name;

	do
	{
		cout << "Welchen Tarif haben sie(2-5): ";
		cin >> d.tarif;
		fehlerCheck();
	}
	while(d.tarif<2 || d.tarif>5);

	do
	{
		cout << "Haben sie die Kanalbuendelung(ja/nein): ";
		cin >> d.kB;
	}
	while(d.kB!="ja" && d.kB!="nein");

	sout.open(F_Kunde.c_str(), ios::app | ios::end | ios::_Nocreate);
	if(sout.good())
	{
		sout << endl << d.id << "\t" << d.name << "\t\t" << d.tarif << "\t\t" << d.kB;
	}
	sout.flush();
	sout.close();
}

void datenAuslesen()
{
	ifstream sin;
	int menue;
	string F_Tarif="tarif.txt",
		   F_Verbrauch="verbrauch.txt";
	string *datei;
	int powerLine;
	double grundGebuehr;
	int freiStunden[2];
	double minuten;

	for(;;)
	{
		do
		{
			system("cls");
			cout << "Willkommen " << d.name << ", waehlen sie ein Menue." << endl << endl
				 << "-----" << endl
				 << " [0] Programm beenden" << endl
				 << " [1] Tarifinfos anzeigen" << endl
				 << " [2] Bisherigen Verbrauch anzeigen" << endl
				 << "-----" << endl << endl
				 << "Menue: ";
			cin >> menue;
		}
		while(menue<0 || menue>2);

		if(menue==0)
		{
			exit(1);
		}
		else if(menue==1 || menue==2)
		{
			d.zNr=0;

			if(menue==1)
			{
				sin.open(F_Tarif.c_str(), ios::in | ios::beg);
			}
			else
			{
				sin.open(F_Verbrauch.c_str(), ios::in | ios::beg);
			}

			if(sin.good())
			{
				do
				{
					if(menue==1)
					{
						sin >> d.zNr >> powerLine >> grundGebuehr >> freiStunden[0] >> freiStunden[1];
					}
					else
					{
						sin >> d.zNr >> minuten;
					}
				}
				while(!sin.eof() && d.id!=d.zNr);
			}
			sin.close();

			system("cls");

			if(menue==1)
			{
				cout << "Ihr Vertrag." << endl << endl
					 << "Tarif: Powerline " << powerLine << endl;

				cout << "Grundgebuehr: " << grundGebuehr << " Euro/Monat" << endl;
				if(d.kB=="ja")
				{
					cout << "Freistunden: " << freiStunden[0] << endl;
				}
				else if(d.kB=="nein")
				{
					cout << "Freistunden: " << freiStunden[1] << endl;
				}
			}
			else
			{
				cout << "Sie waren insgesamt " << minuten << " Minuten im Internet." << endl;
			}
		}
		system("pause");
	}
}

[Firefighter]

Also Tips die ich dir geben kann, nach kurzem überfliegen.
Wenn du std::ifstream benutzt, brauchst du nicht noch explizit beim öffnen angeben das du lesen willst, std::ifstream ist schließlich nur für das lesen gemacht wurden, genauso std::ofstream nur fürs schreiben gemacht wurde.
Weiterhin würde ich dir empfehlen beim prüfen der Datei während des lesens, nicht auf std::fstream::eof() zu prüfen, weil das teilweise zu fehlerverhalten führen kann.
Mach das lieber so hier:

std::ifstream file;

while(getline(file,einstring))//zum lesen von ganzen zeilen
//oder
while(file>>einstring) //zum lesen von wörtern

P.S.Das habe ich schon so oft erzählt, könnte man das ins FAQ pinnen?

[Dravere]

#include <iostream> 
#include <fstream> 
#include <string> 

using namespace std; // Ich rate dringend davon ab.
// using namespace sollte man mit Vorsicht einsetzen.
// Begründungen gibt es viele, da kannst du auch mal im Forum suchen.
// Der Hauptpunkt ist, dass du damit den Sinn von Namensräumen aushebelst.
// In diesem Programm zwar kein Problem, aber man sollte sich sowas nicht
// zur Gewohnheit werden lassen.

struct daten 
{ 
    int id; 
    int zNr; 
    int tarif; 
    string name; 
    string kB; 
}d; // <- ganz schlecht.
// Nicht nur ist es eine globale Variable, welche man vermeiden sollte,
// weil sie die Übersicht des Programms behindern, sondern man sieht es
// hier auch fast nicht. Ein schlecht gewählter Name für eine Variable
// und sehr leicht zu übersehen noch dazu.

void fehlerCheck(); 
void idPrüfen(); // Umlaute in Namen sind so eigentlich per Standard nicht erlaubt.
void idErstellen(); 
void datenAuslesen(); 

const string F_Kunde="kunde.txt"; // Grundsätzlich unnötig. Hier hätte auch
// ein char const* F_Kunde = "kunde.txt"; gereicht.

int main() 
{ 
    string option; 

    do 
    { 
        system("cls");       // Auf system Befehle sollte man verzichten. Sie sind gefährlich,
        system("color 0A");  // Platformabhängig und extrem langsam.

        // Wenn man sowas mit der Farbe oder dem Löschen einbauen möchte, empfiehlt es sich
        // auf Windows die WinAPI zu nehmen. Auf anderen Platformen die entsprechende Schnittstelle.

        // Auch ist ein cls Befehl sehr schlecht. Wenn ich das Konsolenprogramm in einer eigenen
        // Konsole starte, wo ich womöglich Dinge bereits aufgelistet habe, welche ich für das
        // Programm hier brauche, dann möchte ich nicht, dass mir alles weggelöscht wird.
        // Solche Programmierer möchte ich dann am liebsten verstümmeln und dann langsam
        // sterben lassen :)

        cout << "Um das Programm zu beenden geben sie die <0> als ID ein." << endl << endl 
             << "Geben sie ihre Identifikationsnummer ein." << endl 
             << "ID: "; 

        // std::endl sollte man nicht für eine neue Zeile missbrauchen. Ein \n ist nicht nur schneller
        // geschrieben, sondern auch für diesen Zweck vorhanden. std::endl führt nämlich noch ein
        // flush aus.

        // Die folgenden zwei Zeilen sind sehr seltsam. Du liest eine ID ein, in ein Objekt,
        // welches man fast überlesen hätte (im ersten Durchlauf, hatte ich da nicht schlecht
        // gestaunt), und das genialste ist die Funktion danach. Die führt kein fehlerCheck aus,
        // sondern ignoriert alle Fehler und löscht alle entsprechenden Flags.

        // Häää? :D

        cin >> d.id; 
        fehlerCheck(); 

        if(d.id==0) // Empfehlung 0 == d.id -> falls man einen Schreibfehler macht 0 = d.id
        {           // kommt es zu einem Kompilerfehler, andersrum ist dem nicht der Fall.
            exit(0);// exit(0) ist äusserst schlecht. return 0 deutlich besser.
        }           // exit(0) führt zur sofortigen Terminierung. Ressourcen werden nicht frei-
                    // gegeben.

        idPrüfen(); // Diese seltsame Funktion kommentiere ich später.

        if(d.id != d.zNr) // Was? Wofür? Das versteht man erst später, ist aber extrem verwirrend.
        { 
            option=""; // Deutlich einfacher zu lesen wäre option.clear()

            do 
            { 
                system("cls"); // system und cls, wurde schon gesagt 

                cout << "Die eingegebene ID existiert nicht in unserer Datenbank." << endl 
                     << "Moechten sie diese ID erstellen?" << endl 
                     << "(ja/nein): "; 

                cin >> option; // Kein fehlerCheck oder sowas? Gib hier mal das folgende ein:
                // buh bih bah pru ja

                // Der obige Test wird 5 mal ausgegeben, der Input wird nur einmal von dir verlangt.
                // idErstellen wird nach dem 5. Mal ausgeführt.
                // Somit => Unbedingt Stream zurücksetzen, restlichen Inhalt rauswerfen.
                // Oder allenfalls std::getline(std::cin, option); verwenden.
                // Aber auch dann sollte man auf Fehler prüfen.

                if(option == "ja") // Wieder die Empfehlung beides umzukehren "ja" == option.
                { 
                    idErstellen(); 
                    // Grundsätzlich könnte man hier gleich ein break machen, nicht 
                } 
            } 
            while(/*option!="ja" && */option!="nein"); // Dank dem break 
        } 
    } 
    while(option=="nein"); 

    datenAuslesen(); 

    // Grundsätzlich könnte man aber das ganze Ende hier ein wenig umschreiben.
    // Das ganze ein wenig gekürzt dargestellt:

    for(;;)
    {
        // ...

        if(d.id != d.zNr)
        {
            option.clear();

            for(;;)
            {
                // ...

                if("ja" == option)
                {
                    idErstellen();
                    break;
                }
                else if("nein" == option)
                {
                    datenAuslesen();
                    return 0;
                }
            }
        }
    }

    // Allgemeine Empfehlung zu all diesen Verschachtelungen,
    // kommt noch zum Schluss.
} 

void fehlerCheck() 
{ 
    // Ja, diese Funktion, mit völlig falschem Namen.
    // cinErrorClear() wäre sinnvoller gewesen oder
    // flushAndClearInputStream oder was auch immer ...

    cin.clear(); 
    cin.ignore(cin.rdbuf()->in_avail()); 
} 

void idPrüfen() // Umlaut eigentlich nicht erlaubt.
{ 
    // ifstream sin; 
    // sin.open(F_Kunde.c_str(), ios::in | ios::beg); 

    // Die beiden Zeilen kann man Zusammenfassen zu:
    ifstream sin(F_Kunde.c_str());

    // ios::beg ist übrigens kein Flag für den openmode. Das wäre also sogar ein Fehler.

    // Und der folgende Teil ist einfach nur ... hä?
    // Hängt wahrscheinlich auch mit der globalen Variable zusammen.
    // Man hat das Gefühl, dass hier nichts gemacht wird, bzw.
    // eine ganze Menge an Dinge ausgelesen und alles wieder verworfen.

    // Wenn man ein wenig rauf und runter scrollt, bemerkt man
    // das hier nach einem Eintrag mit einer ID gesucht wird.

    // Aber statt dass man der Funktion die ID per Parameter übergibt,
    // wird hier eine globale Variable verwendet. Das ist äusserst unübersichtlich.
    // Auch das schreiben in das globale Objekt ist blöd. Man könnte das Objekt
    // über die Funktion zurückgeben oder eine Referenz auf ein solches Objekt übergeben
    // und dann in dieses übergebene Objekt schreiben.

    // Auch wird der Fehlerfall in dieser Funktion gar nicht behandelt,
    // sondern dann ausserhalb. Und draussen ist man völlig verwirrt,
    // und bemerkt gar nicht, um was es eigentlich geht.
    // Statt dass die Funktion hier ein bool zurückgeben würde,
    // als Resultat, ob der Check erfolgreich war.

    // Der Funktionsname ist allerdings auch verwirrend. idPrüfen prüft nicht nur,
    // sondern liest auch noch Daten ein.

    if(sin.good()) 
    { 
        do 
        { 
            sin >> d.zNr >> d.name >> d.tarif >> d.kB; 
        } 
        while(!sin.eof() && d.id!=d.zNr); 
    } 

    // sin.close(); 
    // Beim verlassen des Scope wird sin zerstört. close() wird dann automatisch
    // aufgerufen.
} 

void idErstellen() 
{ 
    // ofstream sout; Variable wird erst viel später verwendet,
    // kann auch später deklariert werden...

    system("cls"); // Meucheln, Verstümmeln, Töten ... whahahahaha 

    cout << "Bitte geben sie ihre Daten sorgfaeltig ein." << endl << endl; // endl Zeug 
    system("pause"); // system und platform zeug usw. 

    system("cls"); // Meucheln, Verstümmeln, Töten ... whahahahaha  

    cout << "Geben sie ihren Namen ein: "; 
    cin >> d.name; 

    // Und schon wieder wurde es getan. Es wurde eine Funktion aufgerufen,
    // welche im Hintergrund Daten verändert, ohne dass man es wirklich merkt.
    // Globale Objekte sind böse! Nicht ohne Grund kann man Funktionen Parameter übergeben
    // und aus der Funktion Resultate zurückgeben lassen.

    do 
    { 
        cout << "Welchen Tarif haben sie(2-5): "; 
        cin >> d.tarif; 
        fehlerCheck(); // ^^ wäre aber wenn schon besser zu beginn der Schleife.
        // da bei d.name jemand Hans Heintzel eingeben haben könnte. Es wäre nur
        // Hans gelesen worden und Heintzel wäre noch drin.
    } 
    while(d.tarif<2 || d.tarif>5); 

    do 
    { 
        // Kein Fehlercheck?
        cout << "Haben sie die Kanalbuendelung(ja/nein): "; 
        cin >> d.kB; 
        // Kein Fehlercheck?
        // 
    } 
    while(d.kB!="ja" && d.kB!="nein"); 

    // Es wurden schon wieder zwei Variablen global geändert, wovon
    // man anderswo nichts bemerkt hat. Es steht auch nirgends,
    // man hat die Information nur hier.

    // sout.open(F_Kunde.c_str(), ios::app | ios::end | ios::_Nocreate); 

    // ios::end, kein gültiger openmode. ios::_Nocreate auch ungültig, gibt es nicht im Standard.
    // Aber wieso dürfte hier überhaupt kein File erstellt werden? Irgendwie unsinnig.
    // Wenn noch keine Daten vorhanden sind, dann braucht es doch ein File zum rausschreiben.

    // Besser und zusammengefasst:
    ofstream sout(F_Kunde.c_str(), ios::app);

    if(sout.good()) 
    { 
        sout << endl << d.id << "\t" << d.name << "\t\t" << d.tarif << "\t\t" << d.kB; 
    } 

    // Du hast schliesslich mal wieder keine Fehlerbehandlung.

    // Ein close sollte automatisch flushen.
    // sout.flush(); 

    // Und ein close wird automatisch aufgerufen, wenn das Objekt zerstört wird.
    // sout.close(); 
} 

void datenAuslesen() 
{ 
    // Nicht mal mehr im aktuellen C Standard muss man
    // alle Variablen beim Funktionskopf deklarieren. Sowas ist sehr unübersichtlich.
    // Deklariere Variablen dort, wo du sie auch brauchst.

    /*
    ifstream sin; 
    int menue; 
    string F_Tarif="tarif.txt", 
           F_Verbrauch="verbrauch.txt"; 
    string *datei; 
    int powerLine; 
    double grundGebuehr; 
    int freiStunden[2]; 
    double minuten; 
    */

    for(;;) 
    { 
        int menue; // hier brauch wir eine Varialbe.

        do 
        { 
            system("cls");  // Meucheln, Verstümmeln, Töten ... whahahahaha  

            cout << "Willkommen " << d.name << ", waehlen sie ein Menue." << endl << endl 
                 << "-----" << endl 
                 << " [0] Programm beenden" << endl 
                 << " [1] Tarifinfos anzeigen" << endl 
                 << " [2] Bisherigen Verbrauch anzeigen" << endl 
                 << "-----" << endl << endl 
                 << "Menue: "; 

            // endl und so Zeug. Auch wieder die globale Variable.

            cin >> menue; 

            // Wo bleibt die Fehlerüberprüfung? Wenn hier statt einer Zahl ein
            // string eingeben wird, hast du ein Problem.
        } 
        while(menue<0 || menue>2); 

        // switch - case wäre vielleicht auch eine Möglichkeit 

        if(menue==0) // Empfehlung tauschen -> 0 == menue
        { 
            // exit(1); // exit, sehr unschön.
            // wie gesagt, Programm wird sofort terminiert, Ressourcen nicht freigegeben.
            // Ein return wäre hier deutlich klüger. Du kannst die Funktion auch anweisen,
            // dass sie ein int zurückgibt. Und ausserhalb gibst du den int dann weiter.

            // Also hier:
            return 1;
            // Die Funktionsignatur müsste dann lauten: int datenAuslesen()
            // und in der main Funktion rufst du die Funktion so auf:
            // return datenAuslesen();
        } 
        else if(menue==1 || menue==2)  // Empfehlung tauschen
        { 
            d.zNr=0; // wieder diese globale Variable.
            char const* file;

            if(menue==1)  // Empfehlung tauschen
            { 
                // sin.open(F_Tarif.c_str(), ios::in | ios::beg); // <- ios::beg falsch
                file = "tarif.txt";
            } 
            else 
            { 
                // sin.open(F_Verbrauch.c_str(), ios::in | ios::beg);  // <- ios::beg falsch
                file = "verbrauch.txt";
            } 

            // gekürzt:
            ifstream sin(file);

            int freiStunden[2];
            int powerLine; 
            double minuten; 
            double grundGebuehr; 

            if(sin.good()) 
            { 
                do 
                { 
                    if(menue==1) 
                    { 
                        sin >> d.zNr >> powerLine >> grundGebuehr >> freiStunden[0] >> freiStunden[1]; 
                    } 
                    else 
                    { 
                        sin >> d.zNr >> minuten; 
                    } 
                } 
                while(!sin.eof() && d.id!=d.zNr); 
            } 

            sin.close(); 

            system("cls"); // Meucheln, Verstümmeln, Töten ... whahahahaha  

            if(menue==1)  // Empfehlung tauschen
            { 
                cout << "Ihr Vertrag." << endl << endl 
                     << "Tarif: Powerline " << powerLine << endl; 

                cout << "Grundgebuehr: " << grundGebuehr << " Euro/Monat" << endl; 
                if(d.kB=="ja")  // Empfehlung tauschen
                { 
                    cout << "Freistunden: " << freiStunden[0] << endl; 
                } 
                else if(d.kB=="nein")  // Empfehlung tauschen
                { 
                    cout << "Freistunden: " << freiStunden[1] << endl; 
                } 
            } 
            else 
            { 
                cout << "Sie waren insgesamt " << minuten << " Minuten im Internet." << endl; 
            } 

            // Ehm ok, also grundsätzlich ... ehm
            // Was war hier nun gemeinsam zwischen menue == 1 und menue == 2?
            // Ahja, beide haben ein File geöffnet ... das ist alles?
            // Dafür durchmischt du beide Codes und machst die ganze Zeit wieder
            // Fallunterscheidungen rein? Ein bisschen unsinnig nicht?
        } 

        system("pause"); 
    } 

    // Hier müsste noch ein return 0 hin, falls die Funktionsdeklaration so aussähe:
    // int datenAuslesen()
}

In der letzten Funktion habe ich nicht mehr ganz alles kommentiert, vor allem nicht die Dinge, welche ich schon zig Male vorhin erwähnt habe.

Noch ein paar zusätzliche Tipps ganz allgemein:
- Mach mehr Funktionen. Vieles könnte man nochmals in eigene Funktionen auslagern. Das würde auch helfen die tiefen Verschachtelungen wegzubekommen. Oder zumindest würden sie auf Funktionen aufgeteilt werden. Funktionen sollten meistens eher kurz sein und nur eine oder weniger Aufgaben ausführen. Das erhöht auch die Übersicht.
- An der Namensgebung für Variablen und Funktionen solltest du noch arbeiten.
- Keine globalen Variablen!
- Gib deinem Code ein wenig mehr Freiraum. Gerade bei Vergleichen war das Zeug zum Teil ziemlich eng beeinander.

Ich habe nur das Gröbste durchgearbeitet (hat so schon genug Zeit verbraten), aber ich hoffe, dass dir dies schonmal ein wenig weiterhelfen wird.

Grüssli

[GoOber]

erstmal DANKE!!! für deine Mühe^^ .... wie gesagt ich programmiere noch nich lange .... ich sehe ich hab noch viel zu lernen .... und einige dinge die du geschrieben hast kannte ich noch nich .... und vieles wie ich es geschrieben habe wurde mir von der schule beigebracht (z.b. systembefehle) .... aber nochmal danke ich werde das jetzt mal überarbeiten

[Badestrand]

Dravere schrieb:

if(d.id==0) // Empfehlung 0 == d.id -> falls man einen Schreibfehler macht 0 = d.id
            // kommt es zu einem Kompilerfehler, andersrum ist dem nicht der Fall.

Theoretisch eine gute Lösung. Praktisch finde ich's sehr schlecht zu lesen und unnütz: Ich kann mich nicht daran erinnern, je den Fehler ("=" statt "==") gemacht oder beim Debuggen fremden Codes gesehen zu haben.

[Dravere]

Badestrand schrieb:

Dravere schrieb:

if(d.id==0) // Empfehlung 0 == d.id -> falls man einen Schreibfehler macht 0 = d.id
            // kommt es zu einem Kompilerfehler, andersrum ist dem nicht der Fall.

Theoretisch eine gute Lösung. Praktisch finde ich's sehr schlecht zu lesen und unnütz: Ich kann mich nicht daran erinnern, je den Fehler ("=" statt "==") gemacht oder beim Debuggen fremden Codes gesehen zu haben.

Ja, da bist du nicht der einzige, welcher dieser Meinung ist. Ich persönlich habe früher nicht anders gedacht, inzwischen kann ich allerdings beide Versionen gleich gut lesen. Es ist meiner Meinung nach nur eine Gewöhnungssache. Es ist etwas das man von Anfang an so lernt, auch irgendwie intuitiv so macht, weil wir so sprechen und denken.

Und doch, ich habe es schon gesehen, bei mir und bei anderen. Es kommt zwar extrem selten vor, da man meistens höllisch darauf achtet.
Bisher war ich jedenfalls immer sehr froh über den Kompilerfehler, welcher mich dann vor dem Debuggen im Nachhinein verschohnt hat.

Aber deshalb habe ich es auch nur als Empfehlung hingeschrieben. Ein muss ist es definitiv nicht und wer nicht will oder dem es nicht gefällt, der soll es auch nicht tun

Grüssli