Tipps zu meinem C++ code

Was für eine Ausbildung hst du und wieviel zeit hast du gehabt?

Variablen in for-scheifen müssen nicht i und j heißen.

Nachtrag: Ich denke dabei an etwas in dieser Art:

#include <algorithm>
#include <fstream>
#include <iostream>
#include <iterator>
#include <locale>
#include <map>
#include <string>

#include <tr1/functional>

struct string_compare_ci {
public:
  string_compare_ci(std::locale const &loc = std::locale())
    : loc_(loc),
      compare_(std::tr1::bind(char_compare,
                              std::tr1::placeholders::_1,
                              std::tr1::placeholders::_2,
                              loc_)) { }

  static bool char_compare(char c1, char c2, std::locale const &loc) {
    return std::tolower(c1, loc) < std::tolower(c2, loc);
  }

  bool operator()(std::string const &lhs,
                  std::string const &rhs) const {
    return std::lexicographical_compare(lhs.begin(), lhs.end(),
                                        rhs.begin(), rhs.end(),
                                        compare_);
  }

private:
  std::locale loc_;
  std::tr1::function<bool(char, char)> compare_;
};

typedef std::map<std::string, unsigned>                    word_map_t;
typedef std::map<std::string, unsigned, string_compare_ci> word_map_ci_t;
typedef std::pair<word_map_t, word_map_ci_t> word_maps_t;

typedef std::map<std::string, word_maps_t> database_t;

word_maps_t count_words(std::string const &filename) {
  word_maps_t result;
  std::ifstream in(filename.c_str());
  std::string word;

  while(in >> word) {
    ++result.first [word];
    ++result.second[word];
  }

  return result;
}

template<typename map_t>
unsigned word_lookup(map_t const &map, std::string const &word) {
  typename map_t::const_iterator iter = map.find(word);

  return iter == map.end() ? 0 : iter->second;
}

int main(int argc, char *argv[]) {
  if(argc != 2) { return -1; }

  std::ifstream in(argv[1]);

  if(!in) { return -1; }

  database_t database;
  std::string fname;

  while(std::getline(in, fname)) {
    database[fname] = count_words(fname);
  }

  std::string query;

  while(true) {
    std::getline(std::cin, query);

    if(query == "") {
      break;
    }

    unsigned counter_full    = 0;
    unsigned counter_full_ci = 0;

    for(database_t::const_iterator db_iter = database.begin(); db_iter != database.end(); ++db_iter) {
      std::cout << db_iter->first << ":\n";

      unsigned counter    = word_lookup(db_iter->second.first , query);
      unsigned counter_ci = word_lookup(db_iter->second.second, query);

      std::cout << "Case sensitive: "   << counter    << "\n"
                   "Case insensitive: " << counter_ci << '\n';

      counter_full    += counter;
      counter_full_ci += counter_ci;
    }

    std::cout << "Zusammen: \n"
                 "Case sensitive: "   << counter_full    << "\n"
                 "Case insensitive: " << counter_full_ci << std::endl;
  }
}

MiMi85

Erstmal danke, für die vielen schnellen Antworten

Th69 schrieb:

Also ich habe schon deutlich schlechteren Code von C++ Programmierern gesehen.
Ich finde, du hast die Aufgabe gut erfüllt.

Sowas freut einen doch zu hören ;), wo ich doch eher bei Java mich einordne ^^

Th69 schrieb:

Dennoch könnte man noch ein paar Kleinigkeiten anmerken:
- Aufteilung in Input/Output und Logik (d.h. die for-Schleife für die 'map' in eine eigene Methode auslagern)
- Für die Wiederverwendbarkeit des Algorithmus die Funktionen in eine eigene Source- und Header-Datei auslagern

Über eine eigene Header hatte ich nachgedacht, nur habe ich ein wenig gegoogelt, und nirgends steht ab wann man eine header nutzen sollte, nur das es meist sinnvoll ab größeren Programmen mit mehreren Tausend Zeilen wird. Daher hatte ich mich dagegen entschieden, vielleicht ein Fehler -.-

seldon schrieb:

Ich nehme auch an, dass Dinge wie
string fileName = argv[1];          // get name of config file
    ifstream file (fileName.c_str());   // file to read
keinen guten Eindruck gemacht haben, zumal fileName nirgends benötigt wird.

Also lieber direkt

ifstream file (argv[1]);

seldon schrieb:

Außerdem machst du beim Einlesen der Dateien einen Anfängerfehler [...]sonst verarbeitest du die letzte Zeile doppelt. [...] sonst zählst du das letzte Wort doppelt.

Hm, dann ist seltsam, dass das Ergebnis stimmt, also die Ausgabe, wenns doppelt verarbeitet wird.

seldon schrieb:

Was den Algorithmus selbst angeht, wenn du schon eine Datenbank anlegst (was mich vermuten lässt, dass diese nachher oft benutzt werden soll), ist es womöglich effizienter, die Worte gleich beim Einlesen zu zählen und nachher nur noch das Ergebnis nachzuschlagen.

Du meinst mit "Datenbank" die Map? Ja, das mit dem direkt zählen wäre wohl ne Alternative gewesen. Wäre es dann angebracht gewesen, ein eigenes Objekt pro Textdatei zu erstellen, wodrin dann die Informationen wie Wort und Anzahl gespeichert werden? Oder die Map weiter ausbauen
map <string, map <string, int> > (map <dateiname, map <wort, anzahl> >)

krümelkacker schrieb:

Ein unnötiger globaler Zustand (textFiles) ist unschön.

Wie löst man sowas dann am besten? An jede Methode die map mitzugeben, ist doch auch nicht schön oder?

krümelkacker schrieb:

Du hast ein paar unnötige Kopien [...].

Hm, vermutlich aus meinen Java-Kentnissen, einfach alles in ne extra Variable zu speichern, so wie es dort steht ^^ (string curTextFile = i->first;)
Also Referenz auf ne const ->

const string &curTextFile = i->first;

Mit der Referenz wird dann keine Kopie erstellt, weil es nur auf das Objekt zeigt, richtig? Aber warum const, weil es sich nicht verändern soll?

krümelkacker schrieb:

So, wie das Programm aufgebaut ist, speicherst Du alle Wörter aller Dateien. Wenn das so in der Aufgabenstellung drin stand, ok

Ja stand so drin, hier mal der entsprechende Teil:

The program should load all the information it needs on start up.
After start up no file access is allowed, so the program should keep the necessary information in memory.

zzet schrieb:

Was für eine Ausbildung hst du und wieviel zeit hast du gehabt?
Variablen in for-scheifen müssen nicht i und j heißen.

Habe Informatik studiert, hatten aber wie gesagt, hautpsächlich Java.
Das mit i und j ist irgendwie schon immer so gewesen, ka ^^

seldon schrieb:

Nachtrag: Ich denke dabei an etwas in dieser Art:

#include <algorithm>
#include <fstream>
#include <iostream>
#include <iterator>
#include <locale>
#include <map>
#include <string>

#include <tr1/functional>

struct string_compare_ci {
public:
  string_compare_ci(std::locale const &loc = std::locale())
    : loc_(loc),
      compare_(std::tr1::bind(char_compare,
                              std::tr1::placeholders::_1,
                              std::tr1::placeholders::_2,
                              loc_)) { }

  static bool char_compare(char c1, char c2, std::locale const &loc) {
    return std::tolower(c1, loc) < std::tolower(c2, loc);
  }

  bool operator()(std::string const &lhs,
                  std::string const &rhs) const {
    return std::lexicographical_compare(lhs.begin(), lhs.end(),
                                        rhs.begin(), rhs.end(),
                                        compare_);
  }

private:
  std::locale loc_;
  std::tr1::function<bool(char, char)> compare_;
};

typedef std::map<std::string, unsigned>                    word_map_t;
typedef std::map<std::string, unsigned, string_compare_ci> word_map_ci_t;
typedef std::pair<word_map_t, word_map_ci_t> word_maps_t;

typedef std::map<std::string, word_maps_t> database_t;

word_maps_t count_words(std::string const &filename) {
  word_maps_t result;
  std::ifstream in(filename.c_str());
  std::string word;

  while(in >> word) {
    ++result.first [word];
    ++result.second[word];
  }

  return result;
}

template<typename map_t>
unsigned word_lookup(map_t const &map, std::string const &word) {
  typename map_t::const_iterator iter = map.find(word);

  return iter == map.end() ? 0 : iter->second;
}

int main(int argc, char *argv[]) {
  if(argc != 2) { return -1; }

  std::ifstream in(argv[1]);

  if(!in) { return -1; }

  database_t database;
  std::string fname;

  while(std::getline(in, fname)) {
    database[fname] = count_words(fname);
  }

  std::string query;
    
  while(true) {
    std::getline(std::cin, query);

    if(query == "") {
      break;
    }

    unsigned counter_full    = 0;
    unsigned counter_full_ci = 0;

    for(database_t::const_iterator db_iter = database.begin(); db_iter != database.end(); ++db_iter) {
      std::cout << db_iter->first << ":\n";

      unsigned counter    = word_lookup(db_iter->second.first , query);
      unsigned counter_ci = word_lookup(db_iter->second.second, query);

      std::cout << "Case sensitive: "   << counter    << "\n"
                   "Case insensitive: " << counter_ci << '\n';

      counter_full    += counter;
      counter_full_ci += counter_ci;
    }

    std::cout << "Zusammen: \n"
                 "Case sensitive: "   << counter_full    << "\n"
                 "Case insensitive: " << counter_full_ci << std::endl;
  }
}

Dein Level: The Abstraction Freak
http://coderoom.wordpress.com/2010/03/19/5-stages-of-programmer-incompetence/

MiMi85

Achja, Zeitvorgabe gabs keine.

seldon schrieb:

Nachtrag: Ich denke dabei an etwas in dieser Art:

Leider bin ich davon, vom Verständnis und dem Programmieren selbst, wohl noch meilenweit entfernt.

Wir haben halt 2 Jahre Java gemacht und dann ca nen halbes Jahr C++ dran gehängt (Unterricht) und nochmal nen halbes Jahr mit nem eigenen Projekt.

@eeeeeeeke:

Ich bin der Ansicht, dass ein "Abstraction Freak" sich mindestens noch eine Akkumulatorvorlage für die Abfrage geschrieben hätte, der die Zwischenergebnisse in einen eigens für die Formatierung bestimmten Datentyp (d.h. mit entsprechendem operator<< versehenen) verpackt und in einen ostream_iterator geschmissen hätte.

Aber mal in Ernst, wo ist da die große Abstraktion? Eine std::map mit einem eigenen Vergleichsfunktor zu versehen, ist nun wirklich kein besonders komplexer Anwendungsfall, und ansonsten habe ich da gerade mal eine Funktion und eine Funktionsvorlage. Viel einfacher wird das ganze eigentlich nicht mehr.

krümelkacker

MiMi85 schrieb:

krümelkacker schrieb:

Ein unnötiger globaler Zustand (textFiles) ist unschön.

Wie löst man sowas dann am besten? An jede Methode die map mitzugeben, ist doch auch nicht schön oder?

Doch, das ist schon besser. Noch besser sind kleinere Funktionen, die nicht 100 Parameter bekommen. ZB:

long count_word_frequency(vector<string> const& words, string const& word);

MiMi85 schrieb:

krümelkacker schrieb:

Du hast ein paar unnötige Kopien [...].

Hm, vermutlich aus meinen Java-Kentnissen, einfach alles in ne extra Variable zu speichern, so wie es dort steht ^^ (string curTextFile = i->first;)
Also Referenz auf ne const ->
const string &curTextFile = i->first;
Mit der Referenz wird dann keine Kopie erstellt, weil es nur auf das Objekt zeigt, richtig?

Wenn Du schon so fragen musst, ist doch eigentlich klar, dass Du noch nicht wirklich fit in C++ bist. Also: Erstmal die Sprache richtig lernen. Dann das Buch "Effective C++" (3rd edition) lesen.

Aber warum const, weil es sich nicht verändern soll?

Ja, warum nicht? "Const correctness" ist was feines.

Habe Informatik studiert, hatten aber wie gesagt, hautpsächlich Java.

Ging mir auch so. Fand ich im Nachhinein sehr schade. Ich habe mir nach dem Studium C++ selbst beigebracht. Du kannst Die Praktikumsstelle ja annehnen, und dabei C++ lernen. Vielleicht wird das danach doch was mit der Anstellung. Ich würde mir dann aber an Deiner Stelle wirklich 2-3 C++ Bücher besorgen.

kk

MiMi85

Naja eine Praktiktumsstelle war leider nicht verfügbar. Es kam halt nur der Kommentar, dass es so wie es jetzt ist, maximal für eine Praktikumsstelle reichen würde.

Ich hab mich schon öfters umgesehen, nach Tutorials die spezielle für Java-Programmierer sind, zur Umstellung auf C++. Aber selbst dort wird nicht beschrieben, wann zb Pointer gentutz werden (sollten). Oder wann header files. Finde ich sehr schade.
Schaut man sich sonst nach C++ Tutorials um, fangen diese meist ganz am Anfang an, mit "was sind variablen" etc. Das demotivert doch sehr schnell, weil man sich leicht verkaggert vorkommt ^^

krümelkacker

Wie gesagt, ich war in der gleichen Lage. Vergiss Tutorials. Ich hatte TC++PL und EffC++ relativ fix durchgelesen. Es gingen dabei viele Lichter auf, fielen viele Groschen, etc. IMHO ideal für Umsteiger. Wenn's Dir also darum geht, viel komprimierte gute Information in kurzer Zeit aufzunehmen, bist Du mit den zwei Büchern gut beraten. Alles andere kommt dann mit der Zeit.

kk

seldon schrieb:

@eeeeeeeke:

Ich bin der Ansicht, dass ein "Abstraction Freak" sich mindestens noch eine Akkumulatorvorlage für die Abfrage geschrieben hätte, der die Zwischenergebnisse in einen eigens für die Formatierung bestimmten Datentyp (d.h. mit entsprechendem operator<< versehenen) verpackt und in einen ostream_iterator geschmissen hätte.

Aber mal in Ernst, wo ist da die große Abstraktion? Eine std::map mit einem eigenen Vergleichsfunktor zu versehen, ist nun wirklich kein besonders komplexer Anwendungsfall, und ansonsten habe ich da gerade mal eine Funktion und eine Funktionsvorlage. Viel einfacher wird das ganze eigentlich nicht mehr.

Naja, wenn man für so eine Aufgabe bind und function auspackt, sieht mir das schon nach etwas zuviel abstrahiert aus. Und dieser std::locale sieht auch stark danach aus, das ganze irgendwie international zu halten, aber std::string verhindert das wieder.

eeeeeeeke schrieb:

Naja, wenn man für so eine Aufgabe bind und function auspackt, sieht mir das schon nach etwas zuviel abstrahiert aus.

Aber der Rest des Codes mit den maps sieht doch sehr kompakt und funktional aus. Finde ich gut so.

Der große struct am Anfang, nur zum Stringvergleich, sieht schon etwas heftig aus. Den würde ich mindestens auf die Hälfte eindampfen

struct string_compare_ci 
{
private:
  std::locale m_loc;
public:
  string_compare_ci(std::locale const &loc = std::locale()) : m_loc(loc) { }

  bool operator()(const std::string &lhs,
                  const std::string &rhs) const 
  {
      return std::lexicographical_compare(lhs.begin(), lhs.end(),
                                          rhs.begin(), rhs.end(),
                                          [this] (char c1, char c2 ) 
                                          { return std::tolower(c1, m_loc) < std::tolower(c2, m_loc); } );
  }
};

wenn der Compiler sowas schon kann. (Oder sogar versuchen das ganze Ding durch Lambdas zu ersetzen, dann wäre es fast so elegant wie in C#.)

Das ist ein ziemlich merkwürdiges Verständnis von "Abstraktion" - ich benutze ja nur schon vorhandenen Kram. Natürlich hätte ich mir einen eigenen Funktor, der die Locale an die Funktion bindet, schreiben oder std::lexicographical_compare reimplementieren können, aber was hätte das gebracht, von Mehrarbeit abgesehen? Hätte ich string_compare_ci jetzt als Klassenvorlage aufgezogen, die einen Funktortyp zum Vergleichen der normalisierten Zeichen und eine Strategie zur Normalisierung der Zeichen erlaubt hätte, hätte ich die Kritik ja verstanden, aber so, wie's da steht, macht das Zeug schlicht das, was es muss.

Die Locale ist übrigens für die korrekte Behandlung von Umlauten und anderen Sonderzeichen von Bedeutung. Wenn "nämlich" einmal am Satzanfang und einmal in der Satzmitte steht, bist du ohne ziemlich aufgeschmissen.

@nn: Von der Verwendung von Lambda-Ausdrücken habe ich abgesehen, weil der neue Standard noch nicht verabschiedet und dementsprechend nicht von allen gängigen Compilern unterstützt wird. In Produktionscode hätte ich mich aber wohl bei Boost bedient, die haben etwas derartiges schon in der algorithms-Bibliothek.

seldon schrieb:

aber so, wie's da steht, macht das Zeug schlicht das, was es muss.

Ich stimme dir da zu. Gleichzeitig muss ich aber anerkennen, dass solcher Code geeignet ist unsichere Einsteiger in die Flucht zu schlagen.

Aber das ist halt der, etwas kryptische, Charme von C++. Ich kann dem Threadersteller auch nur raten, die genannten Bücher durchzuarbeiten. Dann wird ihm vieles klarer werden.

Und was die Lambdas, functional und bind angeht, vieleicht auch ein Buchtipp. Ich lese gerade "Real world functional programming", ist eigentlich ein Buch über C# und F#, aber danach sieht man manche der neuen Sachen in C++ auch aus einer ganz anderen Perspektive.

nn schrieb:

Ich stimme dir da zu. Gleichzeitig muss ich aber anerkennen, dass solcher Code geeignet ist unsichere Einsteiger in die Flucht zu schlagen.

Da mag was dran sein, aber es hat auch etwas Gefährliches, Anfängern nur Anfänger-C++ zu zeigen. Es ist, und ich bitte, in den Vergleich jetzt nicht zu viel reinzudeuten, ein bisschen wie mit Kindern und Babysprache - sie lernen mehr, wenn man normal zu ihnen spricht.

Es mag durchaus sein, dass ein Anfänger das bind-Konstrukt nicht auf Anhieb durchschaut, aber auf Nachfrage erkläre ich gerne, was das alles bedeutet. Ein Anfänger wird (und muss) nicht alles wissen, aber ich halte es für respektlos, gleich davon auszugehen, dass er es nicht verstehen wird. Ein genaues Verständnis der dahinterliegenden Mechanismen braucht womöglich etwas Erfahrung und weitergehende Kenntnisse, aber mal ganz ehrlich, bevor jemand bis zum Kapitel "Operatorüberladung" gekommen ist, kann er den Mechanismus hinter

std::cout << "Hallo, Welt!" << std::endl;

auch nicht genau verstehen, und die Anwendung von std::tr1::bind und std::tr1::function ist so schwierig eigentlich nicht. Außerdem eröffnen diese Konstrukte einen ganz neuen Blickwinkel auf die Sprache, und es kann nicht schaden, angehende Programmierer damit früh in Berührung zu bringen.

Naja, das ist jedenfalls meine Rechtfertigung. Man kann da wohl geteilter Meinung sein.

seldon schrieb:

Da mag was dran sein, aber es hat auch etwas Gefährliches, Anfängern nur Anfänger-C++ zu zeigen. Es ist, und ich bitte, in den Vergleich jetzt nicht zu viel reinzudeuten, ein bisschen wie mit Kindern und Babysprache - sie lernen mehr, wenn man normal zu ihnen spricht.

Ja das ist auch richtig. Auch sind Programmierer, die von einer anderen Sprache kommen, sicher auch keine Babies.

Aber wenn man sieht, dass der halbe Quelltext in diesem Beispiel eigentlich nur dazu dient etwas zu machen, das woanders z.B. so aussieht

var word_map_ci = new Dictionary<string, string>(StringComparer.CurrentCultureIgnoreCase);

dann bestätigt das doch gewisse Vorurteile über C++. Gut, eigentlich haben wir hier ein Problem des Bibliotheksdesigns, die STL ist mächtig, aber manchmal können kleine Aufgaben komplexen Code erfordern.

[quote="nn"]

seldon schrieb:

Aber wenn man sieht, dass der halbe Quelltext in diesem Beispiel eigentlich nur dazu dient etwas zu machen, das woanders z.B. so aussieht
var word_map_ci = new Dictionary<string, string>(StringComparer.CurrentCultureIgnoreCase);
dann bestätigt das doch gewisse Vorurteile über C++. Gut, eigentlich haben wir hier ein Problem des Bibliotheksdesigns, die STL ist mächtig, aber manchmal können kleine Aufgaben komplexen Code erfordern.

Und das ist das Problem mit Leuten, die nicht genug von C++ verstehen und meinen sich dann drüber beschweren zu müssen. Wäre es anders, wüsstest du das es Quatsch es was du sagst. In C++ kann man das gleiche in einer Zeile machen.
Ich sage nur:

//StringComparer könne genau wie in C# irgendwo in einer Lib definiert sein
std::map<string,string,StringComparer::CurrenCultureIgnoreCase> word_map_ci;

Seldons Beispiel macht deutlich mehr.