"Guter" code?
-
Hallo Leute, wollte mal wissen ob folgendes einem guten Stil entspricht oder was würdet ihr anders machen?
Es ist ein Programm, welches eine Texteingabe speichert und sie in eine Datei speichert. Danach gelangt man zurück ins Menü man kann dieses beenden oder die Datei laden und weiter bearbeiten. Ich verwende außderdem eine map um durch strings in einer switch zu navigieren.#include <fstream> #include <iostream> #include <map> #include <stdlib.h> #include <string> #include "def_function.h" #include <vector> using namespace std; int Save(const vector<string> &save_text, const string save_name) { ofstream save_file; save_file.open(save_name.c_str() , ofstream::app); vector<string>::const_iterator save_iter = save_text.begin(); for(; save_iter != save_text.end(); ++save_iter){ save_file << *save_iter << " "; cout << *save_iter << " wurde gespeichert" << endl; fflush(stdin); } save_file.close(); Menu(); return 0; } vector<string> WordList() { vector<string> wordlist_text; string word; while(cin >> word && word != "EXIT") wordlist_text.push_back(word); return wordlist_text; } void Menu() { cout << "\t\t MENU \t\t\n" << endl; cout << "1. Neue Datei erstellen (new_dat)" << endl; cout << "2. Datei weiter bearbeiten (load_dat)" << endl; cout << "3. Programm schließen (exit)" << endl; map<string, int> choose; choose.insert(map<string, int>::value_type("new_dat", 1)); choose.insert(map<string, int>::value_type("load_dat" , 2)); choose.insert(map<string, int>::value_type("exit", 3)); string acc_choose; cin >> acc_choose; map<string, int>::iterator iter = choose.find(acc_choose); int find_choose = iter->second; switch(find_choose){ case 1:{ CLS; cout << "Bitte geben Sie einen Namen für Ihre Datei an:" << endl; string file_name; cin >> file_name; CLS; cout << "Die Eingabe kann nun begonnen werden:\n" << endl; vector<string> text = WordList(); vector<string>::iterator iter = text.begin(); Save(text, file_name); break; } case 2: { CLS; cout << "Bitte Namen der Datei angeben, die " << "geöffnet werden soll:" << endl; cout << "Nach dme laden der Datei kann die Eingabe" << " fortgesetzt werden.\n" << endl; string load_name; cin >> load_name; ifstream load_file(load_name.c_str()); cout << "\n" << load_file.rdbuf(); vector<string> load_text = WordList(); Save(load_text, load_name); CLS; break; } case 3 : break; } } int main() { cout << "Willkommen zum TextEditor 1.0" << endl; Menu(); return EXIT_SUCCESS; }def_function Header:
#ifndef DEF_FUNCTION_H_INCLUDED #define DEF_FUNCTION_H_INCLUDED // ClearScreen Definition #define CLS system("cls") #include <vector> #include <string> //Funktionen void Menu(); std::vector<std::string> WordList(); int Save(const std::vector<std::string>&, const std::string); #endif // DEF_FUNCTION_H_INCLUDEDDanke für eure Hilfe
-GhostfaceChilla-
-
Ich würde von Grund auf alles anders aufziehen. Derzeit ist es rein prozedurale Programmierung. Genau so würde ein C-Programm aussehen, bloß die Datentypen und vorgefertigten Funktionen würden anders heißen. Der Code nutzt weder generische Programmierung noch Objektorientierung aus, die C++ gegenüber C direkt eingebaut hat. Muss man auch nicht nutzen, ergibt aber meistens besser wartbare und erweiterbare Lösungen, daher würde ich es schon guten Stil nennen.
Für ein funktionales Programm: Es ist ok'isch. Du nutzt an manchen Stellen systemabhängige Funktionalität und ich bin mir bei einigen davon (fflush(stdin)) nicht einmal sicher, ob dir das überhaupt bewusst ist.
Außerdem noch ein paar Kleinigkeiten:
-stdio.h statt cstdio, wie es eigentlich heißt.
-Gemischte Einbindung von Systemheadern und eigenen Headern statt übersichtlich zu trennen
-Überhaupt darf man auch mal Leerzeilen benutzen
-Von den Konstruktoren und Destruktoren eines fstreams scheinst du noch nie gehört zu haben
-Was soll der Rückgabewert von Save?
-Was will das const am zweiten Argument dem Nutze von Save sagen?
-Die Funktionsnamen machen oft nicht klar, was eine Funktion tut.
-Ebenso bei vielen Variablen.
-Bei beiden ist auch die Grammatik der Namen ungewöhnlich. Variablen sind Dinge, (void) Funktionen tun etwas, Funktionen mit Rückgabewert geben ein Ding oder tun etwas. Du benutzt Nomen für Funktionen, Verben für Variablen und ähnliches. Sehr verwirrend.
-Makros
-Du hast oft temporäre Variablen, die in einer Zeile definiert, in der nächsten zugewiesen, in der dritten benutzt werden und dann nie wieder vorkommen.
-Wozu ist eigentlich der Header, wenn alles eine Übersetzungseinheit ist?Nicht so Kleinigkeiten:
-Save/Menu ist ja rekursiv!
Ich denke da hast du einen ganz dicken Fehler gemacht
-Gib mal ungültige/unerwartete Daten ein!Abschließend muss ich zurück nehmen: Nein, das ist nicht ok. Die "Nicht so Kleinigkeiten" sind zu krass und die "Kleinigkeiten" zu viele.
-
Ok, vielen dank ich werde mal alles überarbeiten.
-GhostafceChilla-
-
#ifndef DEF_FUNCTION_H_INCLUDED #define DEF_FUNCTION_H_INCLUDED // ClearScreen Definition // #define CLS system("cls") <- kotz, graus, würg ;) #include <vector> #include <string> // Funktionen // <- Offensichtliches kommentiert man nicht. // void Menu(); bool Menu(); std::vector<std::string> WordList(); // int Save(const std::vector<std::string>&, const std::string); void Save( std::vector< std::string > const &text, std::string const &filename ); #endif // DEF_FUNCTION_H_INCLUDED<- wofür eigentlich der Header?
#include <fstream> #include <iostream> #include <map> #include <cstdlib> // #include <stdlib.h> #include <string> #include <vector> #include <limits> // numeric_limits #include <Windows.h> #undef max // Gaaahrrch MAKROS :/ #include "def_function.h" // nicht-Standard-Header nach den Standard-Headern using namespace std; void cls() // siehe unten { COORD origin = { 0, 0 }; DWORD chars_written; CONSOLE_SCREEN_BUFFER_INFO csbi; DWORD console_size; HANDLE std_out = GetStdHandle( STD_OUTPUT_HANDLE ); GetConsoleScreenBufferInfo( std_out, &csbi ); console_size = csbi.dwSize.X * csbi.dwSize.Y; FillConsoleOutputCharacter( std_out, (TCHAR) ' ', console_size, origin, &chars_written ); GetConsoleScreenBufferInfo( std_out, &csbi ); FillConsoleOutputAttribute( std_out, csbi.wAttributes, console_size, origin, &chars_written ); SetConsoleCursorPosition( std_out, origin ); } // int Save( const vector< string > &save_text, const string save_name ) // Wofür der Prefix "save_" bei den Parametern? // Warum int zurückgeben wenn eh immer 0 zurückgegeben wird? // Warum den Dateinamen nicht auch als Referenz? void Save( vector< string > const &text, string const &filename ) { // ofstream save_file; // Schon wieder der sinnfreie Präfix ... // file.open(save_name.c_str() , ofstream::app); // Warum in einen C-String wandeln? // Warum nicht den Konstruktor verwenden? ofstream file( filename, ofstream::app ); // vector<string>::const_iterator save_iter = save_text.begin(); // warum nicht im for-Statement? for( vector< string >::const_iterator i = text.begin(); i != text.end(); ++i ) { file << *i << " "; cout << *i << " wurde gespeichert" << '\n'; // << endl; <- ist in den seltensten Fällen nötig. // fflush(stdin); warum stdin während der ausgabe flushen? } // file.close(); <- macht der Destruktor am ende des Scopes // Menu(); <- Wirklich!? Irgendwann ist der Stack voll ... // return 0; <- Immer dieselbe Konstante zurückgeben bringt bei dieser Funktion nix. } vector< string > WordList() { vector<string> wordlist_text; string word; while( cin >> word && word != "EXIT" ) wordlist_text.push_back( word ); return wordlist_text; } // void Menu() bool Menu() // gibt false zurück, wenn das Programm beendet werden soll. { cout << "\t\t MENU \t\t\n\n"; // << endl; <- siehe oben cout << "1. Neue Datei erstellen (new_dat)\n"; // << endl; <- siehe oben cout << "2. Datei weiter bearbeiten (load_dat)\n"; // << endl; <- siehe oben cout << "3. Programm schließen (exit)\n"; // << endl; <- siehe oben map< string, int > choose; choose.insert(map<string, int>::value_type("new_dat", 1)); choose.insert(map<string, int>::value_type("load_dat" , 2)); choose.insert(map<string, int>::value_type("exit", 3)); string acc_choose; cin >> acc_choose; // map<string, int>::iterator iter = choose.find(acc_choose); // int find_choose = iter->second; switch( choose[ acc_choose ] ){ case 1: { // CLS; <- ein Makro für einen system()-call? Zwei undinge vereint ;) cls(); cout << "Bitte geben Sie einen Namen für Ihre Datei an:\n"; // << endl; <- siehe oben string file_name; cin >> file_name; cls(); cout << "Die Eingabe kann nun begonnen werden:\n\n"; // << endl; <- siehe oben vector<string> text = WordList(); // vector<string>::iterator iter = text.begin(); <- wofür genau? Save(text, file_name); break; } case 2: { // CLS; <- oben. cls(); // cout << "Bitte Namen der Datei angeben, die " << // "geöffnet werden soll:" << endl; // cout << "Nach dme laden der Datei kann die Eingabe" << // " fortgesetzt werden.\n" << endl; // also ein bisserl breitere Displays hamma inzwischen schon ... cout << "Bitte Namen der Datei angeben, die geöffnet werden soll:\n"; // << endl; <- siehe oben cout << "Nach dme laden der Datei kann die Eingabe fortgesetzt werden.\n\n"; // << endl; <- siehe oben string load_name; cin >> load_name; // ifstream load_file(load_name.c_str()); // <- siehe oben. ifstream load_file( load_name ); cout << "\n" << load_file.rdbuf(); vector<string> load_text = WordList(); Save(load_text, load_name); // warten, sonst siehst nix. cin.ignore( numeric_limits< streamsize >::max(), '\n' ); // alles verwerfen was vielleicht noch gepuffert ist cin.get(); // CLS; <- oben. cls(); break; } case 3: // break; return false; } return true; // <- weiter geht's ;) } int main() { cout << "Willkommen zum TextEditor 1.0\n"; // << endl; <- siehe oben // Menu(); <- nicht nur einmal: while( Menu() ); // return EXIT_SUCCESS; <- unnötig. main() gibt implizit 0 (aka EXIT_SUCCESS) zurück, wenn sie nichts zurückgibt. }
-
Viele dank auch
Das einzigste was ich nicht kapier ist die void cls() Funktion...
Ah und hab gedacth man soll nur dann Parameter zu Referenzen machen, wenn es sich auch lohnt, sprich bei containern etc. welche auch eine immense größe annehmen kann. Und habe gedacht das man strings nicht benutzen kann als Dateipfad in streams sondern nur C-Style strings??-GhostfaceChilla-
-
GhostfaceChilla schrieb:
Das einzigste was ich nicht kapier ist die void cls() Funktion...
Die ruft nur ein Paar WinAPI-Funktionen auf. Dokumentation dazu ist die MSDN.
GhostfaceChilla schrieb:
Ah und hab gedacth man soll nur dann Parameter zu Referenzen machen, wenn es sich auch lohnt, sprich bei containern etc. welche auch eine immense größe annehmen kann.
std::stringist ein Container.GhostfaceChilla schrieb:
Und habe gedacht das man strings nicht benutzen kann als Dateipfad in streams sondern nur C-Style strings??
ISO/IEC 14882:2011(E) schrieb:
27.9.1.7 Constructors:
basic_ifstream(); explicit basic_ifstream(const char* s, ios_base::openmode mode = ios_base::in); explicit basic_ifstream(const string& s, ios_base::openmode mode = ios_base::in); basic_ifstream(const basic_ifstream& rhs) = delete; basic_ifstream(basic_ifstream&& rhs);
-
#include <limits> #include <vector> #include <string> #include <fstream> #include <iostream> class token_editor_t { friend std::istream& operator>> ( std::istream &, token_editor_t & ); friend std::ostream& operator<< ( std::ostream &, token_editor_t const & ); private: bool ready; bool changed; std::string name; std::vector< std::string > tokens; public: typedef std::vector< std::string >::size_type size_type; token_editor_t( ) : ready( false ), changed( false ) { } bool is_ready() const { return ready; } bool is_changed() const { return changed; } bool is_empty() const { return tokens.empty(); } std::string const & get_filename() const { return name; } size_type size() const { return tokens.size(); } void open( std::string const &filename ) { reset(); std::fstream file( filename, std::ios::in ); if( file.good() ) { std::string token; while( file >> token ) tokens.push_back( token ); ready = true; name = filename; } } void save() { std::fstream file( name, std::ios::out | std::ios::trunc ); for( std::vector< std::string >::const_iterator i = tokens.begin(); i != tokens.end(); ++i ) file << *i << ' '; changed = false; } std::string remove( size_type index ) { std::string token = tokens[ index ]; tokens.erase( tokens.begin() + index ); changed = true; return token; } void reset() { ready = false; changed = false; name = ""; tokens.clear(); } }; std::istream& operator>> ( std::istream &is, token_editor_t &editor ) { std::string token; is >> token; editor.tokens.push_back( token ); editor.changed = true; return is; } std::ostream& operator<< ( std::ostream &os, token_editor_t const &editor ) { for( std::vector< std::string >::size_type i = 0; i != editor.tokens.size(); ++i ) os << " [" << i+1 << "] " << editor.tokens[ i ] << '\n'; return os; } int main() { bool resume = true; token_editor_t token_editor; do { std::cout << "Token Editor:\n\n [1] create\n [2] open\n [3] list\n [4] add\n [5] remove\n [6] save\n [7] discard\n [8] exit\n\n > "; int choice = std::cin.get(); std::cout.put( '\n' ); bool create = false; std::string filename; switch( choice ) { case '1': // create create = true; case '2': // open; if( token_editor.is_changed() ) { std::cout << "There are unsaved changes. Please save or discard first!"; break; } std::cout << "Please enter the name of the file to be " << ( create ? "created" : "opened" ) << ": "; std::cin >> filename; token_editor.open( filename ); break; case '3': // list if( !token_editor.is_empty() ) std::cout << token_editor; else std::cout << "Editor is empty!"; break; case '4': // add if( token_editor.is_ready() ) { std::cout << "New token: "; std::cin >> token_editor; std::cout << "Token added."; } else std::cout << "No file created or opened!"; break; case '5': // remove if( !token_editor.is_empty() ) { std::cout << token_editor; std::cout << "\n\nItem to remove: "; token_editor_t::size_type delpos = 0; if( std::cin >> delpos && delpos && delpos + 1 <= token_editor.size() ) std::cout << "\nToken \"" << token_editor.remove( --delpos ) << "\" has been removed."; else std::cout << "Invalid index!"; } else std::cout << "Editor is empty!"; break; case '6': // save if( token_editor.is_changed() ) { token_editor.save(); std::cout << "Changes saved to \"" << token_editor.get_filename() << '\"'; } else std::cout << "No changes made!"; break; case '7': // discard if( token_editor.is_changed() ) { token_editor.reset(); std::cout << "Changes discarded."; } else std::cout << "Nothing to discard!"; break; case '8': // exit resume = false; break; } std::cout << "\n\n"; std::cin.ignore( std::numeric_limits< std::streamsize >::max(), '\n' ); } while( resume ); }Selbe Frage wie Ghostface ( == "Ihr dürft mich jetzt steinigen."
)
-
SeppJ schrieb:
-Save/Menu ist ja rekursiv!
Ich denke da hast du einen ganz dicken Fehler gemacht Nein, das ist übersichtlicher.
http://www.c-plusplus.net/forum/307401
