programmierstiel, stolperstiene und fehlersicherheit



  • Hallo Forum,

    ich habe in den letzten Tagen einen ini parser geschrieben.
    Dabei habe ich versucht diesen so zu entwerfen, das die Einträge im file konfig.ini auf Fehler überprüft werden könn.
    Da die Sektions und die in den Sektions vorkommenden Keys bekannt sind habe ich Dabei auf auf die stl map zurück gegriffen und ein System entworfen mit dem ich prüfen kann ob es sich bei dem gelesenen zeile um eine gültige Section bzw. einen gültigen Key innerhalb einer bestimmten Section handelt.
    Dazu benutze ich 2 zusätzliche ini Dateien, in denen jeweils die Sections sowie alle Keys pro Section hinterlegt sind. Diese werden dann ausgelesen und in die Map eingetragen. Da diese Files nur selten geändert werden müssen, werden diese einfach zeile für zeile ausgelesen.(keine inhaltliche fehler prüfung, da nicht möglich). durch dieses System erhoffe ich mir das ich die map sehr einfach um weitere Devicetypen(und damit keyMaps) erweitern kann.

    Nun wo ich das ganze am laufen habe würde ich euch bitten da einmal drüber zu schauen und mir mitzutielen was ihr davon haltet. Ich stelle diese Frage desshalb weil ich gerne anregungen, verbesserungen und kritik zum dem Modul haben würde. Also was ist gut was nicht was habe ichgemacht sollte es aber (besser)nicht machen und wieso?

    Ich habe das Projekt mittels eclipse unter Linux geschrieben und werde es als zip hochladen.
    http://www.file-upload.net/download-8938686/ini_Parser.zip.html

    hier schon mal einen fettes Dankeschön an alle die mir hier helfen mich zu verbessern.



  • Ich habe mal einen kurzen Blick drauf geworfen und mir sind einige Sachen aufgefallen, die merkwürdig aussehen:

    - die Klassen devConfig und devBox erwecken den Anschein, sie seien nur eine Ansammlung von Daten und erfüllen selbst keine weitere Funktion. Dann habe ich gesehen, dass sie von TimerIntervall abgeleitet sind. Sieht für mich nach einem Designfehler aus, außer es gibt einen Grund, den ich nicht kenne. Wenn es reine Datenhaltungs-Klassen sind reicht da struct völlig aus.

    - du benutzt keine Initialisierungslisten, um die Member deiner Klassen zu initialisieren. Stattdessen werden im Konstruktor Zuweisungen gemacht.

    - du benutzt in allen Methoden explizit this->, um auf Member zuzugreifen. Ist eine Stilfrage, macht den Code in meinen Augen aber schwerer zu lesen.

    - du benutzt an einigen Stellen eigene Include-Guards, statt das die einzelnen Include Dateien selbst regeln zu lassen (zB boxConfig.h).

    - du benutzt einige Setter-Methoden, die einen konstanten Wert zurückgeben (zB sämtliche Setter in boxConfig.cpp). Wenn diese Methoden immer 0 als Konstante zurückgeben brauchst du keinen Rückgabewert.

    - die Datei Konfig.ini besitzt Tags, die das Ende eines Abschnitts kennzeichnen sollen (zB [dev1] und [/dev1]). Normale .ini Dateien kennen solche Abschnittsenden nicht, für mich sieht das nach einer XML-Anleihe aus. Wenn du XML machen willst, nimm XML, wenn du INI machen willst bleib bei den gängigen Konventionen.

    - die Logik hinter parser.cpp und parsermap.cpp habe ich nicht ganz verstanden, habe allerdings auch nicht lange drauf geguckt. Es hat aber den Anschein, dass du mit beiden Klassen konkret gegen ein Ziel programmiert hast, nämlich dass du in der INI Datei nur bestimmte Objekte mit genau definierten Eigenschaften und Namen ablegen kannst. Damit ist der INI-Parser zu eng mit den Datenstrukturen verwoben, idealerweise sollte ein INI-Parser überhaupt nicht wissen, welche konkreten Datentypen in der INI Datei vorhanden sind. Ein gut designter INI-Parser sollte den Inhalt einer halbwegs komfortabel anbieten, zB als map<std::string, vector<std::pairstd::string,std::string>>. Damit liegt hinter jedem Abschnitt ein Vektor mit Schlüssel/Wertepaaren. Wenn die Reihenfolge der Abschnitte wichtig wird musst du aber wohl auf eigene Datenstrukturen ausweichen, zB:

    #include <string>
    #include <vector>
    
    struct Tuple
    {
       std::string Key;
       std::string Value;
    };
    
    struct Section
    {
       std::string Name;
       std::vector<Tuple> Items;
    };
    
    std::vector<Section> read_ini_file( const std::string& FileName );
    
    // von mir aus auch als Implementation durch Stream Operatoren
    istream& operator>>( istream& is, std::vector<Section>& rhs );
    ostream& operator<<( ostream& os, const std::vector<Section>& rhs );
    

    Damit kannst du das Datenablageformat (.INI) sauber von deinen Programmdaten trennen, das Einlesen geschieht völlig unabhängig von der Bedeutung der Daten.

    Das Einlesen erfolgt dann in zwei Schritten, das Speichern analog umgekehrt

    1. Einlesen der INI Datei und Bereitstellung als std::vector<Section>.
    2. Parsen des Vektors und Erzeugen der entsprechenden Daten für dein Programm aus den Daten des Vektors

    und

    1. Erzeugen des Vektors aus den Programmdaten
    2. Schreiben des Vektors

    Fazit:
    Dein Programm läuft irgendwie und mag funktionieren, aber es ist schlecht designed und inkonsistent. Du hast feste Bindungen zwischen Programmteilen (Programmdaten und Datenablage), wo keine sein sollten. Du scheinst dir irgendwas vorgestellt zu haben, was dein Programm können soll (zB erweiterte Schlüsselwörter/Hierarchien in der INI Datei), hast die Gedanken aber nicht zu Ende gebracht. Übrig geblieben ist spezialisierter Spaghetti Code, der nur genau für einen Anwendungsfall funktioniert. INI Parser würde ich das nicht nennen, obwohl irgendwo irgendwann eine INI Datei gelesen wird.



  • hi DocShoe,
    Zu aller erst einmal Danke für deine mühe.
    Ich werde mir deine Anmerkungen zu herzen nehmen und diverse Teile noch einmal überarbeiten.
    Nebenher möchte ich auch noch auf ein par deiner Anmerkungen antworten und erleäutern was ich gemacht habe und wiso ich es so gemacht habe.
    Evtl. wird einuíges dadurch klarer(?).

    Also:

    DocShoe schrieb:

    die Klassen devConfig und devBox erwecken den Anschein, ...

    Du hast recht die Beiden Klassen sind im Prinzip reine Datenhaltungs Objekte.
    Ich habe diese um die Funktion von timerIntervall erweitert.(dabei handelt es sich um eine Klasse welche einem Timer bereitstellt. (gibt true zurück wenn das Intervall abgelaufnen ist). Diese Funktionalität wird nur in der Hauptanwendung benötigt. In der Hauptanwendung gibt es jeweils ein objekt pro zu lesendem Device. dieses erbt jeweils von einem der besagten Datenobjekten (je nach typ boxConfig oder devConfig) und damit auch den benötigten Timer.
    Sollte ich hier evtl. direkt die Klassen dev und BoxConfig von timerintervall ableiten.

    DocShoe schrieb:

    du benutzt an einigen Stellen eigene Include-Guards,

    Da hast du recht. ICh werde alle meine header überarbeiten. #pragma once ist hier das stichwort oder?

    DocShoe schrieb:

    die Datei Konfig.ini besitzt Tags, die das Ende eines Abschnitts kennzeichnen sollen (zB [dev1] und [/dev1])

    Auch hier hast du nicht ganz unrecht. Mein erster ansatz wahr genau das. Alles mittels xml zu hinterlegen. jedoch hatt xml eine ganze menge Overhead welcher in dieser speziellen Anwendung nicht benötigt wird. Also hab ich mich entschieden einen anderen weg zu gehen. Du hast Recht das normaler weise keine explizieten Ende-tags vorhanden sind. Hier stelle ich mir jedoch die frage warum ist das so und wieso sollte man das nicht machen? Eigentlich ist es doch so wesentlich verständlicher wann und wo das ende kommt außerdem kann ich so prüfen ob ggf. ein erwartetes ende nicht kommt (also ein Fehler in der Datei vorliegt).

    DocShoe schrieb:

    die Logik hinter parser.cpp und parsermap.cpp habe ich nicht ganz verstanden,

    Hier ist es so das es relativ Wichtig ist das alle Sektionen und Keys auch vorhanden sind. Ansonsten wird evtl ein Device(Sensor etc.) nicht initialisiert und es kommt in der Hauptanwendung zu einem Verbindungsfehler oder ggf. sogar zu Hardwareschäden (recht unwarscheinlich aber sicher ist sicher).
    Deshalb solte hier auch das Programm abgebrochen werden. (bevor etwas an der hardware engestellt wird).
    Dabie spielt zwar die Reihnefolge der Sektionen und der Keys innerhalb der Sektionen keine große Rolle solange sie eben da sind. (einzige ausnahme ist der DeviceTyp, der der erster Eintrag jeder Sektion. Dieser eintrag dient als such Schlüssel für die secktionMap).
    Deshalb hatte ich die Idee mit der KeyMap. Dem liegt zu Grunde, das jedes Device(z.B. sensor etc.) seine ganz eigenen Eigenschaften hatt.
    so braucht DeviceTyp 1 z.B. zwei Steuerregister Devicetyp 2 hingegen keine, Device 1 speichert die messwerte als float in zweie 16 bit registern, Device 1 als Integer in einem 32 bit Register, etc... dies ist je nach hersteller recht unterschiedlich.
    Mittels der gelesenen keyMap kann ich nun Prüfen ob entsprechend des gelsenen DeviceTyps(dient zum auswählen der keyMap) die richtigen Keys gelesen werden und kann diese der gewünschten Set-Funktion des datenObjektes(BoxConfig o. DevConfig) zuordnen.
    (ich nehme mal an, das genau diese feste Zuweisung nicht besonders elegant ist oder?)
    Dabei setzte ich voraus, dass die Dateien für die sectionMap.ini und die map.ini richtig sind. Dies setze ich voraus, da diese Dateien auch bei späteren Betrieb nur für ein Update zur verwendung von neuer Hardware geändert(ergänzt) werden müssen. Darüberhinaus werden alle Prüfungen welche den Inhalt von konfig.ini betreffen dynamisch aus den anderen beiden Files (sectionMap.ini und map.ini) erzeugt.

    Ich hoffe das jetzt etwas klarer geworden ist was ich eigentlich machen wollte. 😃

    Wenn ich deine Kritik richtig aufgenommen habe, sollte ich also erst die komplete ini Datei einlesen, in einem Vektor + map speichern und erst dann die Prüfung der richtigkeit der Daten vornemen.


Log in to reply