Datei auslesen und hineinschreiben

Fuchs aus dem Wald

Hey Leute,
ich melde mich auch mal wieder.
Ich würde gerne mal etwas Kritik für eine Funktion von mir ernten! Ob das so ok ist oder ob ihr noch Verbesserungsvorschläge habt. Bitte nicht zu high end! Ich bin immer noch Anfänger.
Ach so und eine Frage habe ich auch:
Ist perror ok? Ich hab für C++ nur so etwas gefunden:

std::cerr << std::strerror(errno) << '\n';

Hab das nicht ausprobiert da mir perror sehr gefällt.

bool EnvironmentSettings::WriteConfig(const char *file){
  std::fstream config_file;
  config_file.open(file, std::ios::in | std::ios::out);
  if(!config_file){
    perror("Config file not load!");
    return false;
  }
  std::string str_env_begin = "#exenv";
  std::string str_env_end = "#endexenv";
  std::vector<std::string> buffer;
  std::vector<std::string>::iterator start_pos;
  std::vector<std::string>::iterator end_pos;

  //read config and delete all environment variables
  std::copy(std::istream_iterator<std::string>(config_file), std::istream_iterator<std::string>(), std::back_inserter(buffer));
  config_file.close();
  start_pos = std::find(buffer.begin(), buffer.end(), str_env_begin);
  end_pos = std::find(buffer.begin(), buffer.end(), str_env_end);
  if(start_pos != buffer.end()){
    buffer.erase(start_pos, (end_pos+1));
  }

  //writing new config
  std::fstream new_config_file;
  new_config_file.open("/etc/envtmp", std::ios::out);
  if(!new_config_file){
    perror("New config file not load!");
    return false;
  }
  for(auto iter = buffer.begin(); iter != buffer.end(); iter++){
    new_config_file << *iter;
  }

  new_config_file << std::endl << str_env_begin;
  for(EnvData *curenv = FirstEnvData; curenv; curenv = curenv->Next){
    if(curenv->Type == ENVVALUE_STRING){
      new_config_file << std::endl << curenv->Key << " = " << (char*)curenv->Value;
    }else{
      new_config_file << std::endl << curenv->Key << " = " << *(int*)curenv->Value;
    }
  }
  new_config_file << std::endl << str_env_end;

  new_config_file.close();

  //delete old config file and rename the tmp file
  std::remove(file);
  int result = rename("/etc/envtmp", file);
  if(result != 0){
    perror("Error renaming file");
    return false;
  }

  return true;
}

Werner Salomon

Fuchs aus dem Wald schrieb:

//delete old config file and rename the tmp file
  std::remove(file);

Interessant - scheint es tatsächlich zu geben. Ein Erbe aus alten C-Tagen

Fuchs aus dem Wald

Wie geht es besser udn was ist falsch? Bei mir baut das und es macht was es soll. Und ja ich weiß, dass das nicht heißt das es auch gleich das macht was ich denke.
Ich werde mir das noch einmal genauer anschauen.

Werner Salomon schrieb:

Fuchs aus dem Wald schrieb:
//delete old config file and rename the tmp file
  std::remove(file);
Interessant - scheint es tatsächlich zu geben. Ein Erbe aus alten C-Tagen

Hä, diese remove Funktion liegt doch gar nicht im namespace std?

Fuchs aus dem Wald

Kann es sein das es mit und ohne std klappt?

http://www.cplusplus.com/reference/algorithm/remove/

http://www.cplusplus.com/reference/cstdio/remove/

@TE:
-Du solltest RAII nutzen.
-perror und strerror ist eher C like.
-ich würd ein bisschen mit typedef arbeiten, und die langen Typen nicht immer ausschreiben.
-Das erase hat glaub eine Reallokation zurfolge. Das konnte man doch mit erase-remove-idiom verhindern.
-Wieso sind da std::endl und C-Casts?

Fuchs aus dem Wald schrieb:

Kann es sein das es mit und ohne std klappt?

http://www.cplusplus.com/reference/algorithm/remove/

http://www.cplusplus.com/reference/cstdio/remove/

Eigentlich ::removeremove(file);

out schrieb:

Fuchs aus dem Wald schrieb:

Kann es sein das es mit und ohne std klappt?

http://www.cplusplus.com/reference/algorithm/remove/

http://www.cplusplus.com/reference/cstdio/remove/

Eigentlich ::removeremove(file);

::remove(file);

Fuchs aus dem Wald

out schrieb:

@TE:
-Du solltest RAII nutzen.
-perror und strerror ist eher C like.
-ich würd ein bisschen mit typedef arbeiten, und die langen Typen nicht immer ausschreiben.
-Das erase hat glaub eine Reallokation zurfolge. Das konnte man doch mit erase-remove-idiom verhindern.
-Wieso sind da std::endl und C-Casts?

Danke erst einmal dafür.
1. Kenne ich nicht vielleicht magst du dazu was sagen werde aber auch mal nachschauen.
2. Ok das strerror auch eher C-like ist wusste ich nicht. Gibt es denn dafür etwas anderes in C++?
3. Du meinst:

typedef std::vector<std::string>::iterator vec_it;

oder?
4. Werde ich einmal nachschlagen.
5. std::endl mach ich raus. Für die C-Casts muss ich mich entschuldigen.

Was mach ich nun mit dem remove?

Leider grad total wenig Zeit. Hab mal auf die schnelle einen Anfang gemacht, würde bei mir wohl irgendwie so aussehen.

bool EnvironmentSettings::WriteConfig( const string& file )
{
  ifstream config_file( file.c_str() );
  if( !config_file.is_open() )
  {
    cerr << "Config file not load!";
    return false;
  }

  typedef vector<std::string> container;
  typedef container::iterator iter;
  container buffer;
  copy( istream_iterator<string>(config_file), istream_iterator<string>(), back_inserter(buffer));

  string str_env_begin = "#exenv";
  string str_env_end = "#endexenv";

  iter start_pos = find(buffer.begin(), buffer.end(), str_env_begin);
  iter end_pos   = find(buffer.begin(), buffer.end(), str_env_end);

  if(start_pos != buffer.end())
  {
    	buffer.erase(start_pos, (end_pos+1));
  }

  //writing new config
  ofstream new_config_file( "/etc/envtmp" );
  if( !new_config_file.is_open() )
  {
    cerr << "New config file not load!";
    return false;
  }

  for( const auto& s : buffer )
  {
    new_config_file << s;
  }

Fuchs aus dem Wald

Danke out für die Anregungen. Hab einiges umgesetzt. Beschäftige mich nun mit dem erase noch einmal.
Hat sonst noch jemand was?

Werner Salomon

Fuchs aus dem Wald schrieb:

Danke out für die Anregungen. Hab einiges umgesetzt. Beschäftige mich nun mit dem erase noch einmal.
Hat sonst noch jemand was?

Beim Kopieren der Daten in Zeile 31 solltest Du hinter jedem Wort noch ein Leerzeichen oder Linefeed einfügen, sonst wird Wort an Wort hinter einander in die Datei geschrieben.
Weiter halte ich es für sinnvoll, dass man den std::vector<std::string> buffer einspart und die gelesenen Wörter gleich in die Zieldatei schreibt - weniger Code weniger Speicher und wahrscheinlich performanter. Und drittens würde ich für die Klasse/struct EnvData einen Ausgabeoperator schreiben, dann wird der Code noch etwas einfacher - etwa so:

bool EnvironmentSettings::WriteConfig( const char *file )
{
    using namespace std;
    {
        ifstream config_file( file );
        if( !config_file.is_open() )
        {
            cerr << "Config file not load!";
            return false;
        }
        ofstream new_config_file( "/etc/envtmp" );
        if( !new_config_file.is_open() )
        {
            cerr << "New config file not load!";
            return false;
        }

        const string str_env_begin = "#exenv";
        const string str_env_end = "#endexenv";
        bool copy_ = true;
        for( string word; config_file >> word; )
        {
            if( copy_ )
            {
                if( word == str_env_begin )
                    copy_ = false;
                else
                    new_config_file << word << "\n";
            }
            else if( word == str_env_end )
                copy_ = true;
        }

        new_config_file << str_env_begin << "\n";
        for(EnvData *curenv = FirstEnvData; curenv; curenv = curenv->Next){
            new_config_file << *curenv << "\n";
        }
        new_config_file << str_env_end << std::endl;
    } // hier sind die Dateien wieder zu

    //delete old config file and rename the tmp file
    std::remove(file);
    int result = rename("/etc/envtmp", file);
    if(result != 0){
        cerr << "Error renaming file" << endl;
        return false;
    }
    return true;
}

Gruß
Werner

Fuchs aus dem Wald

Wow vielen dank Werner!
Das sieht super aus.
Ich schau mal ob ich das mit dem Ausgabeoperator noch hin bekomme.

Ach so und noch eine Frage:
Durch den

using namespace std

scope werden dann die Dateien geschlossen, wenn dieser sich schließt? Wenn ich den nicht benutzen würde müsste ich vorher

file.close()

machen richtig? Nur das ich das richtig verstehe.

Ach was ich grade sehe der Scope gehört gar nicht zu den namespace.
Gibt es einen Namen für das was du dort angewendet hast? Also das ist doch einfach nur ein Scope oder?

volkard

Fuchs aus dem Wald schrieb:

Also das ist doch einfach nur ein Scope oder?

Ja. Einfach, damit die Datei nach Gebrauch schnell genug geclosed wird, vor dem remove.
Da mit den ganzen Exceptions und dem Mitten-Aus-Der-Funktion-Herausreturnrn (Zeile9 und 15) man eh aufs explizite Hinschreiben von close völlig verzichtet und das den Destruktor automatisch machen lässt, ist das mit dem scope statt close halt der konsequente Weg.

Fuchs aus dem Wald

@volkard
Danke für die Erklärung.
Ist das schlecht, wenn man da mit raus returned? Sollte ich das anders lösen?

volkard

Fuchs aus dem Wald schrieb:

@volkard
Danke für die Erklärung.
Ist das schlecht, wenn man da mit raus returned? Sollte ich das anders lösen?

Das WAR mal schlecht, damals als Strukturierte Programmierung die heilige Kuh war, die jeder Lehrer anbetete. Als man noch close() selber hinschreiben musste, als es keine sofort zuschlagenden Destruktoren gab.

Aber jetzt isses gut! Sobald der Returnwert der Funktion feststeht, sofort raus. Das macht alles einfacher.

Fuchs aus dem Wald

Ok danke für das Hintergrundwissen. Wieder was gelernt.