Problem mit funktion für ini files

[burnner]

Hallo,
ich habe mir eine Klasse programmiert um ini files zu editieren:

//////////////////////////////////
//add a key
bool iniOfStream::addKey(std::string key, std::string value,std::string section)
{
    //variables
    char buffer[1000];
    std::string temp;
    std::string text;
    std::string text_2;
    int pos;

    this->ini_file_two.open(this->name.c_str());//open ini file

    while(this->ini_file_two.getline(buffer, 1000))
    {
       text += "\n";//new line
       text += buffer;//copy buffer into text
       temp = buffer;

       pos = temp.find(section);//search section

       if(pos != std::string::npos)//found!
       {
          //add key with value
          text += "\n";
          text += key;
          text += "=";
          text += value;
       }

    }
    text.erase(0,1);
    this->ini_file_two.close();//close ini file

    this->ini_file.open(this->name.c_str());//open ini file
    this->ini_file << text << std::endl;//write ini file
    this->ini_file.close();
    return true;
}
//////////////////////////////////
//add a key
bool iniOfStream::editKey(std::string key, std::string value,std::string section)
{
    //variables
    char buffer[1000];
    std::string temp;
    std::string text;
    bool found = false;
    bool done = false;
    int pos;

    this->ini_file_two.open(this->name.c_str());//open ini file

    while(this->ini_file_two.getline(buffer, 1000))
    {
       temp = buffer;

       if(found && !done)//search key
       {
          pos = temp.find(key);

          if(pos!=std::string::npos)//key found!
          {
             //add new value
             pos = temp.find("=");
             temp.erase(pos+1, temp.length()-1);
             temp += value;
             done = true;
          }
       }

       if(!found && !done)
       {
          pos = temp.find(section);//search section

          if(pos != std::string::npos)//found!
          {
               found =true;
          }
       }
       text += "\n";//new line
       text += temp;//copy buffer into text

    }
    text.erase(0,1);

    this->ini_file_two.close();//close ini file

    this->ini_file.open(this->name.c_str());//open ini file
    this->ini_file << text << std::endl;//write ini file
    this->ini_file.close();
    return true;
}
//////////////////////////////////

Wen ich einen Schlüssel ändere, funktioniert alles, wen ich jedoch schnell hintereinander einige Schlüssel ändere, ist die Ini datei danach leer

findet da jemand den fehler?

mfg burnner

[HumeSikkins]

Hallo,
1. Deine Kommentare sind vollständig nutzlos. Das ist schade. Wenn du dir schon die Mühe machst Kommentare zu schreiben (gute Sache), dann solche, die einen Zewck erfüllen (z.B. Vor- und Nachbedingungen von Funktionen, Erklärungen *warum* Code das tut was er tut). Alles andere (Kommentarre die nur wiederholen, was der Code bereits sagt) ist sinnlos und völlig kontraproduktiv. Noch schlimmer wird es, wenn die Kommentare lügen (siehe add a key über Methode editKey).
Ein gute Ort für ein Kommentar wäre z.B. die Stelle wo du text.erase(0, 1) machst. Dort löschst du das erste Zeichen von text. Warum?
Auch interessant: Warum schreibst du das Newline-Zeichen *bevor* du die gelesene Zeile einfügst?

2. Benutzerdefinierte Typen wie std::string solltest du per Referenz-auf-const übergeben nicht per Value.

3. Definiere Variablen erst dort wo du sie brauchst und erst dort wo du sinnvolle Anfangswerte für sie hast.

4. Gib Variablen sprechende Namen. Namen die ausdrücken, was der Sinn einer Variablen ist. Variablennamen wie buffer, temp, text oder text_2 sagen gar nichts. Das wird zu einem Problem, wenn deine Methoden länger werden. So wie die geposteten.

5. Verwende die Stringversion von getline statt der char*-Version. Statt:

char buffer[1000];
ini_file_two.getline(buffer, 1000)

lieber

string buffer;
getline(ini_file_two, buffer)

6. Vermeide unnötige Variablen. temp und pos erfüllen in addKey keinen sinnvollen zweck.

7. Prüfe auf Fehler. In diesem Fall solltest du zimndest prüfen, ob die Dateien korrekt geöffnet wurden.

8. Vermeide unnötigen Code.

if(!found && !done)

Die Prüfung auf !done ist hier überflüssig. done kann in deinem Code nur seinen Wert ändern, wenn zuvor found seinen Wert ändert.

bool iniOfStream::addKey(const std::string& key, const std::string& value, const std::string& section)
{
    ini_file_two.open(name.c_str());
    if (ini_file_two.is_open())
    {
	    std::string newContent;
	    for (std::string line; std::getline(ini_file_two, line);)
	    {
	    	newContent += "\n";
	    	newContent += line;
	    	if (line.find(section) != std::string::npos)
	    	{
	    		newContent += "\n";
	          	newContent += key;
	          	newContent += "=";
	          	newContent += value;
	    	}
	    }
	    newContent.erase(0,1);	// Warum?
	    ini_file_two.close();

		ini_file.open(name.c_str());
    	if (ini_file.is_open())
    	{
    		ini_file << newContent << std::endl;
    		ini_file.close();
    		return true;
    	}
	}
    return false;

}

bool iniOfStream::editKey(const std::string& key, const std::string& value, const std::string& section)
{
	ini_file_two.open(name.c_str());
	if (ini_file_two.is_open())
	{
	    std::string newContent;
	    bool sectionFound = false;
	    bool valueChanged = false;
	    for (std::string line; std::getline(ini_file_two, line);)
	    {
	    	if (!sectionFound && line.find(section) != std::string::npos)
	    	{
	    		sectionFound = true;
	    	}
	    	if (sectionFound && !valueChanged && line.find(key))
	    	{
	    		// Der Code (genau wie deiner) ist unsicher, da er den 
	    		// Sonderfall "="-nicht-gefunden nicht berücksichtigt.
	    		line.replace(temp.find("="), std::string::npos, value);
	    		valueChanged = true;
	    	}	
	    	newContent += "\n";
	    	newContent += line;
	    }
	    newContent.erase(0,1);
   		ini_file_two.close();

	    ini_file.open(name.c_str());
	    if (ini_file.is_open())
	    {
	    	ini_file << newContent << std::endl;
	    	ini_file.close();
	    	return true;
	    }
	}
	return false;
}

So. Das sind mal die Änderungen die du imo auf jeden Fall machen solltest. Ist aber natürlich nur ein Anfang. Beide Methoden sind noch viel zu lang, sollten also sinnvoll zerlegt werden. Außerdem sollten beide noch deutlich robuster werden.

Das sagt dir zwar alles noch nicht wo jetzt genau der Fehler ist, aber vielleicht hilft es dir ja schonmal indirekt. Ansonsten musst du das mit dem "schnell hintereinander" nochmal etwas genauer erklären.

PS: Achtung: Nicht kompilierter Code

[Rocketmaster5000]

Ich hab zu drei eine Frage:

Ich hab das bisher auch immer so gemacht das ich Varaiblen am Anfange einer Methode / Funktion initialisiert habe, damit man gleich sieht "was Sache ist".

Natürlich bin ich immer bemüht besser zu werden Hast du das aus eigenen Erfahrungen abgeleitet das es besser ist sie erst dann zu deklarieren und zu initalisieren wenn man sie braucht? Oder gibts dazu einen kleinen Lesetip?

[Sovok]

Rocketmaster5000 schrieb:

Ich hab zu drei eine Frage:

Ich hab das bisher auch immer so gemacht das ich Varaiblen am Anfange einer Methode / Funktion initialisiert habe, damit man gleich sieht "was Sache ist".

Natürlich bin ich immer bemüht besser zu werden Hast du das aus eigenen Erfahrungen abgeleitet das es besser ist sie erst dann zu deklarieren und zu initalisieren wenn man sie braucht? Oder gibts dazu einen kleinen Lesetip?

da scheiden sich die geister
nach meiner erfahrung neigen c programmierer eher dazu alle variablen am anfang zu definieren, während c++ leute es erst reinschreiben wenn sies brauchen

such dir eine variante aus, bleib aber konsequent

[Cocaine]

Naja, einen Unterschied gibt's schon:

std::string s; // Konstruktor wird aufgerufen
// irgendwas, nur s wird nicht benutzt
s = "hallo"; // operator= wird aufgerufen

oder

// irgendwas, nur s wird nicht benutzt
std::string s = "hallo"; // entsprechender Konstruktor wird aufgerufen

Inwiefern der Compiler ersteres allerdings optimiert, was ich mir vorstellen könnte, weiß ich nicht.

[Sovok]

oder

std::string s ( "hallo" );

[nman]

Cocaine schrieb:

Inwiefern der Compiler ersteres allerdings optimiert, was ich mir vorstellen könnte, weiß ich nicht.

Das dürfte meines Erachtens eher unwahrscheinlich sein, wenn Du die Wahl zwischen Initialisierung und Zuweisung hast, entscheide Dich immer für Initialisierung.

Sovok: Variablendeklarationen am Anfang eines Blocks sind IMO eine C89-Krankheit, die man aber in C++ auf keinen Fall übernehmen sollte.

Rocketmaster5000:
Lesetip(p)

[rocketmaster5000]

nman schrieb:

Cocaine schrieb:

Inwiefern der Compiler ersteres allerdings optimiert, was ich mir vorstellen könnte, weiß ich nicht.

Das dürfte meines Erachtens eher unwahrscheinlich sein, wenn Du die Wahl zwischen Initialisierung und Zuweisung hast, entscheide Dich immer für Initialisierung.

Sovok: Variablendeklarationen am Anfang eines Blocks sind IMO eine C89-Krankheit, die man aber in C++ auf keinen Fall übernehmen sollte.

Rocketmaster5000:
Lesetip(p)

hm So ganz überzeugt das nun nicht. Unbewusst hab ich das nämlich schon immer so gemacht (Built in variablen am anfang, objekte / instanzen erst wenn man sie braucht).

Wieso verallgemeinerst du dann in deinem Post und schreibst das "Variablendeklarationen" am Anfang eines Blocks eine Krankheit sind?

Der Autor deines Lesetipps macht das so, aber halt nur bei built ins.

[burnner]

Ey keine kritik an meinen Kommentaren^^ naja das mit dem lügen und den unnötigen variabeln kommt vom umstellen etc^^. Aber !Done brauchts 100% ohne das wird es nicht gehen.. naja wen ich zwei mal hintereinander sofort einen schlüssel ändere gehts nicht..

newContent.erase(0,1);    // Warum?  Weil am Anfang immer ein Zeilen umbruch eingefügt wird^^ weiss zwar auch wieso^^

So herausfinden warum das nicht geht und dann wieder ne neue funktion schreiben.. ich Denke es liegt daran, weil ich die Textdatei öffne schliesse öffne...

[HumeSikkins]

burnner schrieb:

Aber !Done brauchts 100% ohne das wird es nicht gehen.

Ach so? Na dann erklär mal.
Ich sehe das so:
* Du hast zwei lokale boolesche-Variablen found und done, die beide mit false initialisiert werden.

* Der erste Block wird betreten, wenn found == true und done == false. Von der Ausgangssituation betrachtet fehlt also ein found = true. Merken wir uns.

* Der zweite Block wird betreten, wenn found == false und done == false. Von der Ausgangssituation betrachtet fehlt also nix. Der zweite Block wird auf jeden Fall im ersten Durchlauf betreten.

* done wird nur an einer Stelle verändert. Nämlich im ersten Block. Dort wird es auf true gesetzt. Minimale Bedingung von der Ausgangssituation: found = true.

* Wie kriegen wir found auf true? Nur im zweiten Block.
Minimale Bedingung von der Ausgangsituation. Keine.

Schauen wir uns also alle möglichen Kombinationen an:
found, done:
false, false: Möglich: -> Zweiter Block wird betreten, erster nicht
false, true: Unmöglich: -> done kann nur im ersten Block auf true gesetzt werden. Dazu ist aber found == true nötig.
true, fals: Möglich -> Zweiter Block wird nicht betreten, erster ja
true, true: Möglich -> Zweiter Block wird nicht betreten, erster nicht.

So. Wie du feststellen solltest, ist die Prüfung auf !done für den zweiten Block unnötig.

Aber warum verschwende ich eigentlich meine Zeit für jemanden der scheinbar sowieso völlig kritikunfähig ist???
Naja, hilf dir am Besten selbst.