Tester gesucht/Vorstellung - KISS Ebook - Ebook Schnellstarter [Linux]

Wutz

Ich habe nicht das Programm getestet - nur den Code aus kisslib.c mal durchleuchtet:

- gtk.h (also Oberflächenzeugs) gehört nicht in eine Daten-Lib
- "while !feof()" ist ein Anfängerfehler
- "ftell-SEEK END" ist auch fehleranfällig + unpraktisch, besser ist stat
- bei fopen immer "rb"
- besser statt des Rumoperierens auf FILE ist es, das ganze File in den Speicher zu laden und dann ausschließlich dort zu arbeiten (auch bei großen Files ist das deiner Variante vorzuziehen)
- eine Daten-Lib sollte keine Ausgaben auf stdout machen, im Fehlerfall auf stderr - besser noch wäre ein kleines Fehlerframework um Konsolenausgaben ganz zu vermeiden
- Kandidaten für Überläufe sind vorprogrammiert (im wahrsten Sinn):

char attributeBuffer[1024];
...
  char xRef[64];

sowas macht man mit symbol. Konstanten und verwendet diese Größenangaben natürlich dann später wieder

int readUntil = atol(xRef);

unsauber und fehlerintolerant (atol liefert long und nicht int, atol liefert 0 bei Fehlern,...)

- es gäbe noch vieles mehr zu bemängeln, ich mache hier erstmal Schluss
- insgesamt viel zu viel Code für ein simples Datei-Lesen/Auswerten

hustbaer

Wutz schrieb:

- Kandidaten für Überläufe sind vorprogrammiert (im wahrsten Sinn):
char attributeBuffer[1024];
...
  char xRef[64];
sowas macht man mit symbol. Konstanten und verwendet diese Größenangaben natürlich dann später wieder

Da bin ich - wenn ich dich richtig verstanden habe - nicht ganz deiner Meinung.

MMn. sollte man überall wo man die Array-Grösse braucht diese mit sizeof() ermitteln. Da man natürlich nicht überall 100x das sizeof(arr)/sizeof(arr[0]) wiederholen möchte halt per Makro - ARRAY_SIZE(arr) oder so. Das reduziert die Fehleranfälligkeit nochmal, da man so nicht mehr die falsche symbolische Konstante für ein Array verwenden kann. (Natürlich kann man immer noch ARRAY_SIZE(a) machen und dann auf Array b zugreifen, aber da hier der Bezeichner der selbe ist - bzw. sein sollte, fällt das mMn. leichter auf.)

Symbolische Konstanten kann und sollte man natürlich trotzdem machen. Bloss halt nicht überall im Code verwenden um die Grösse zu ermitteln.

theSplit

Hallo,

Code Anregungen sind natürlich auch gern gesehen, man lernt ja nicht aus.. daher möchte ich gern dazu Stellung nehmen warum und wieso.

Wutz schrieb:

Ich habe nicht das Programm getestet - nur den Code aus kisslib.c mal durchleuchtet:

- gtk.h (also Oberflächenzeugs) gehört nicht in eine Daten-Lib

Beim Auslesen von PDFs kann es vorkommen, dass das UI nicht mehr reagiert, um dem aus dem Weg zu gehen, wird in der Lib die GTK Hauptroutine aufgerufen, damit das Interface responsive bleibt.

- "while !feof()" ist ein Anfängerfehler

Du irrst in diesem Fall, es ist sogar sehr nützlich. Während die Prüfung auf "\0" bei fgetc(inputFile) eine "\0" zurückgeben kann, selbst wenn wir uns in der Datei befinden, nutze ich "!feof()".

Ansonsten beendet die Routine innerhalb der Datei, obwohl wir noch gar nicht dort sind wo wir sein sollen.

- "ftell-SEEK END" ist auch fehleranfällig + unpraktisch, besser ist stat

Okay, das muß ich mir ansehen. Gebe ich dir Recht und werde ich auch machen.

- bei fopen immer "rb"

Was wäre der Vorteil bzw. der Unterschied zu einem normalen Einlesen mittels "r" ? Magst du mir das erklären?

- besser statt des Rumoperierens auf FILE ist es, das ganze File in den Speicher zu laden und dann ausschließlich dort zu arbeiten (auch bei großen Files ist das deiner Variante vorzuziehen)

Widerspreche ich dir, weil man eine 20 MB PDF nicht vorladen lassen muß, nur um in den ersten 1,5 MB aufhören zu lesen, finde ich jedenfalls nicht sinnvoll.

- eine Daten-Lib sollte keine Ausgaben auf stdout machen, im Fehlerfall auf stderr - besser noch wäre ein kleines Fehlerframework um Konsolenausgaben ganz zu vermeiden

Für das Debugging oder das Troubleshooting von Bug-Reports aber unerlässlich. Ich schreibe keine Log Files mit Fehlern, daher ist diese Variante sehr viel leichter zu implementieren und man kann den Fehler auch sehr gut nachvollziehen.

- Kandidaten für Überläufe sind vorprogrammiert (im wahrsten Sinn):
char attributeBuffer[1024];
...
  char xRef[64];
sowas macht man mit symbol. Konstanten und verwendet diese Größenangaben natürlich dann später wieder

Nein, weil, wenn du schaust, der Buffer wird beim erreichen der Grenze von 1024 Bytes geleert bzw. auf 0 Position gesetzt. Es kann also daher aktuell kein Buffer Overflow auftreten. Es kann aber auch gut sein das ich dies später hinzugefügt habe.

int readUntil = atol(xRef);
unsauber und fehlerintolerant (atol liefert long und nicht int, atol liefert 0 bei Fehlern,...)

Mit dem Long stimmt, so sollte dies auch sein - selbst wenn der Wert 0 ist, wird eine Alternative angeboten wenn nichts bekannt ist.
Was wäre deiner Meinung nach die beste Alternative zu ermitteln, bis zu welchem Punkt eine PDF ausgelesen werden soll? - Es gibt, meines Wissens nach, keine fixe Dateiposition, außer dieser Xref.

- es gäbe noch vieles mehr zu bemängeln, ich mache hier erstmal Schluss
- insgesamt viel zu viel Code für ein simples Datei-Lesen/Auswerten

Wenn du gezielt in den Bytes umherspringen kannst, und auch die Werte bekommst, nur zu, ich würde mich über Unterstützung freuen.

-------------

hustbaer schrieb:

Da bin ich - wenn ich dich richtig verstanden habe - nicht ganz deiner Meinung.

MMn. sollte man überall wo man die Array-Grösse braucht diese mit sizeof() ermitteln. Da man natürlich nicht überall 100x das sizeof(arr)/sizeof(arr[0]) wiederholen möchte halt per Makro - ARRAY_SIZE(arr) oder so. Das reduziert die Fehleranfälligkeit nochmal, da man so nicht mehr die falsche symbolische Konstante für ein Array verwenden kann. (Natürlich kann man immer noch ARRAY_SIZE(a) machen und dann auf Array b zugreifen, aber da hier der Bezeichner der selbe ist - bzw. sein sollte, fällt das mMn. leichter auf.)

Nur, damit ich es auch verstehe, man kann bei einem String Array sizeof verwenden um die Größe zu bestimmen, ohne den Weg über Strlen zu gehen?

Symbolische Konstanten kann und sollte man natürlich trotzdem machen. Bloss halt nicht überall im Code verwenden um die Grösse zu ermitteln.

Ja, das wäre in der Tat empfehlenswert, so hätte jede Funktion ein, einmalig gesetzes, Limit, an dem man sich orientieren kann.

Aber wie gesagt, ich teste auf den gleichen Wert, muß man sich ohne Konstante natürlich merken und abändern, das dieser nicht überschritten wird.

Ich geb aber zu, das wäre, als Limit, definitiv eine bessere Wahl im Code und würde auch eine zentrale Stelle dafür bieten, und nicht überall verteilt etwas was es auch abzuändern gibt.

--------

Vielen Dank für das Feedback

hustbaer

theSplit schrieb:

hustbaer schrieb:

Da bin ich - wenn ich dich richtig verstanden habe - nicht ganz deiner Meinung.

MMn. sollte man überall wo man die Array-Grösse braucht diese mit sizeof() ermitteln. Da man natürlich nicht überall 100x das sizeof(arr)/sizeof(arr[0]) wiederholen möchte halt per Makro - ARRAY_SIZE(arr) oder so. Das reduziert die Fehleranfälligkeit nochmal, da man so nicht mehr die falsche symbolische Konstante für ein Array verwenden kann. (Natürlich kann man immer noch ARRAY_SIZE(a) machen und dann auf Array b zugreifen, aber da hier der Bezeichner der selbe ist - bzw. sein sollte, fällt das mMn. leichter auf.)

Nur, damit ich es auch verstehe, man kann bei einem String Array sizeof verwenden um die Größe zu bestimmen, ohne den Weg über Strlen zu gehen?

Nö. Du kannst damit die Grösse des Arrays ermitteln. strlen gibt dir ja zurück wie lange der darin gespeicherte "String" ist. Ganz was anderes.

theSplit

@hustbaer

Ja gut, das macht dann auch Sinn.

Also um es in anderen Worten zu sagen: Im Grunde erhält man dann den Wert, der an Speicher für das Objekt (hier Char Array) reserviert wurde...

Hätte ich auch selbst drauf kommen können, eigentlich.

Aber vielen Dank für die Klärung.

SeppJ

theSplit schrieb:

Beim Auslesen von PDFs kann es vorkommen, dass das UI nicht mehr reagiert, um dem aus dem Weg zu gehen, wird in der Lib die GTK Hauptroutine aufgerufen, damit das Interface responsive bleibt.

Und wenn der Nutzer deiner allgemeinen Bibliothek kein GTK benutzt? Das gehört nicht in die Bibliothek, sondern den Anwendungscode.

Du irrst in diesem Fall, es ist sogar sehr nützlich. Während die Prüfung auf "\0" bei fgetc(inputFile) eine "\0" zurückgeben kann, selbst wenn wir uns in der Datei befinden, nutze ich "!feof()".

Ansonsten beendet die Routine innerhalb der Datei, obwohl wir noch gar nicht dort sind wo wir sein sollen.

Du verstehst nicht, wie eof funktioniert. Es ist ein Logikfehler in deinem Programm, ein typischer Anfängerfehler. Da gibt es keine Ausrede, du hast das Problem nicht einmal verstanden, weil du nicht weißt, wie man in C überhaupt korrekt Eingaben validiert.

- bei fopen immer "rb"

Was wäre der Vorteil bzw. der Unterschied zu einem normalen Einlesen mittels "r" ? Magst du mir das erklären?

"r" interpretiert die Daten als wären es menschenlesbare, zeilenbasierte Textdaten, "rb" gibt die Rohdaten zurück (Konkret geht es um Zeilenumbrüche, aber es ist nicht garantiert, dass dies alles ist). Da EPUB ein Maschinenformat ist, ist ungeheuer wichtig, dass man an den Rohdaten nichts verandert. Du hast wahrscheinlich nur unter Linux-artigen Systemen getestet, wo "r" und "rb" zufällig das gleiche sind, sonst wäre der Unterschied sofort aufgefallen.

Widerspreche ich dir, weil man eine 20 MB PDF nicht vorladen lassen muß, nur um in den ersten 1,5 MB aufhören zu lesen, finde ich jedenfalls nicht sinnvoll.

Stimmt, sonst liefe das Programm nicht auf meinem 386er...

- eine Daten-Lib sollte keine Ausgaben auf stdout machen, im Fehlerfall auf stderr - besser noch wäre ein kleines Fehlerframework um Konsolenausgaben ganz zu vermeiden

Für das Debugging oder das Troubleshooting von Bug-Reports aber unerlässlich. Ich schreibe keine Log Files mit Fehlern, daher ist diese Variante sehr viel leichter zu implementieren und man kann den Fehler auch sehr gut nachvollziehen.

Du versteht wieder das Problem nicht. Wieso stdout? Eine Bibliothek hat niemals etwas auf stdout auszugeben (außer die stdio-Bibliothek), denn da gehören Ausgaben des Anwendungscodes hin. Wozu sind denn deiner Meinungnach stderr und stdlog da, wenn nicht zur Dokumentation vor Error- und Logdaten?

- Kandidaten für Überläufe sind vorprogrammiert (im wahrsten Sinn):
char attributeBuffer[1024];
...
  char xRef[64];
sowas macht man mit symbol. Konstanten und verwendet diese Größenangaben natürlich dann später wieder
Nein, weil, wenn du schaust, der Buffer wird beim erreichen der Grenze von 1024 Bytes geleert bzw. auf 0 Position gesetzt. Es kann also daher aktuell kein Buffer Overflow auftreten. Es kann aber auch gut sein das ich dies später hinzugefügt habe.

Du verstehst schon wieder das Problem nicht. Du musst an jeder Stelle im Programm genau wissen, dass dieses Array 1024 groß ist. Wenn es sich jemals ändert, musst du genau alle Stellen im Programm ändern, wo es benutzt wird. Eine Stelle vergessen und schon kracht es.

int readUntil = atol(xRef);
unsauber und fehlerintolerant (atol liefert long und nicht int, atol liefert 0 bei Fehlern,...)
Mit dem Long stimmt, so sollte dies auch sein - selbst wenn der Wert 0 ist, wird eine Alternative angeboten wenn nichts bekannt ist.
Was wäre deiner Meinung nach die beste Alternative zu ermitteln, bis zu welchem Punkt eine PDF ausgelesen werden soll? - Es gibt, meines Wissens nach, keine fixe Dateiposition, außer dieser Xref.

Du verstehst schon wieder das Problem nicht. Du machst keine vernünftige Fehlerbehandlung. Du kannst gar keine vernünftige Fehlerbehandlung machen, da atol dies gar nicht zu lässt. Woher willst du wissen, ob ein Wert ungültig war oder ob es eine echte 0 war? Nimm die richtigen Funktionen für diese Aufgabe, also die strtol & co.

Du scheinst insgesamt eher da dran interessiert, deinen schlechten Code zu verteidigen, als dich ernsthaft mit der Kritik auseinander zu setzen. Wutz hat das nicht geschrieben, um dich persönlich anzugreifen, sondern um zu zeigen, was an dem Code falsch/schlecht ist. Vielleicht fehlt dir auch einfach das Wissen, um seine Kritik richtig zu verstehen.

theSplit

Hallo SeppJ,

ich werte Wutz und Hustbaers Bemerkungen keineswegs als Angriff. Ich finde es ja gut wenn man mir versucht Tips zu geben wie ich etwas verbessern kann.

Aber dann würde ich auch darum bitten, nach Möglichkeit, Anregungen zu geben was man wie verbessern kann und Fragen dazu auch zu beantworten. Damit ich den Fehler wirklich beheben und das Problem in der Zukunft vermeiden kann.

Und dann tue ich das auch sehr gern.

Und dann Frage ich halt gern auch noch einmal nach, weil mir manches eben nicht so einfach klar ist.

Nun aber zu deinen Punkten:

SeppJ schrieb:

theSplit schrieb:

Beim Auslesen von PDFs kann es vorkommen, dass das UI nicht mehr reagiert, um dem aus dem Weg zu gehen, wird in der Lib die GTK Hauptroutine aufgerufen, damit das Interface responsive bleibt.

Und wenn der Nutzer deiner allgemeinen Bibliothek kein GTK benutzt? Das gehört nicht in die Bibliothek, sondern den Anwendungscode.

Ich verstehe das Problem diese "Logik" von der "Datenverarbeitung" zu trennen und würde generell auch nicht widersprechen wollen, allerdings würde gerade diese eine Ausleseroutine das UI blockieren können. Eine andere Möglichkeit die mir dann in den Sinn kommen würde, wäre Threading zu verwenden so das die Leseroutine "parallel" im Hintergrund laufen könnte, aber auch das UI "responsive" gehalten wird.

Ich habe nur leider auch festgestellt das diverse Aufrufe zu "gtk_main_iteration()" dazu führen, das die Leseroutine massiv ausgebremst wird - jedenfalls ohne eine Art von Threads zu verwenden.

Eine andere Idee hätte ich pauschal sonst nicht dies anders zu lösen.

Du irrst in diesem Fall, es ist sogar sehr nützlich. Während die Prüfung auf "\0" bei fgetc(inputFile) eine "\0" zurückgeben kann, selbst wenn wir uns in der Datei befinden, nutze ich "!feof()".

Ansonsten beendet die Routine innerhalb der Datei, obwohl wir noch gar nicht dort sind wo wir sein sollen.

Du verstehst nicht, wie eof funktioniert. Es ist ein Logikfehler in deinem Programm, ein typischer Anfängerfehler. Da gibt es keine Ausrede, du hast das Problem nicht einmal verstanden, weil du nicht weißt, wie man in C überhaupt korrekt Eingaben validiert.

Nun, darum hab ich ja geschrieben was ich weiß, damit man mich auch korrigieren kann.

Ich dachte "EOF" hat als Konstante auch den generischen Wert "0" ?

- bei fopen immer "rb"

Was wäre der Vorteil bzw. der Unterschied zu einem normalen Einlesen mittels "r" ? Magst du mir das erklären?

"r" interpretiert die Daten als wären es menschenlesbare, zeilenbasierte Textdaten, "rb" gibt die Rohdaten zurück (Konkret geht es um Zeilenumbrüche, aber es ist nicht garantiert, dass dies alles ist). Da EPUB ein Maschinenformat ist, ist ungeheuer wichtig, dass man an den Rohdaten nichts verandert. Du hast wahrscheinlich nur unter Linux-artigen Systemen getestet, wo "r" und "rb" zufällig das gleiche sind, sonst wäre der Unterschied sofort aufgefallen.

Da hast du Recht, in der Tat habe ich ausschließlich unter Linux getestet und, das hatte mich auch verwundert, keinen Unterschied festgestellt ob ich nun "r" oder "rb", "rb" was ich anfangs sogar verwendet hatte weil ich eben auch dachte "es handelt sich doch um einen binären Datenstrom"... das es nun "zufällig" identisch ist, war mir nicht klar.

Ändere ich ab.

Widerspreche ich dir, weil man eine 20 MB PDF nicht vorladen lassen muß, nur um in den ersten 1,5 MB aufhören zu lesen, finde ich jedenfalls nicht sinnvoll.

Stimmt, sonst liefe das Programm nicht auf meinem 386er...

Ich weiß nicht welche technische Relevanz diese "Erklärung" haben soll es liefe nicht auf einem 386er..., dass es von Nachteil sein könnte, alle Daten einzulesen um mit einem kleinen Anteil dieser zu arbeiten, rein von der Performance her steht aber schon noch im Raum als Gedanke oder?
Es geht mir hier nicht rein um den Speicherverbrauch.

- eine Daten-Lib sollte keine Ausgaben auf stdout machen, im Fehlerfall auf stderr - besser noch wäre ein kleines Fehlerframework um Konsolenausgaben ganz zu vermeiden

Für das Debugging oder das Troubleshooting von Bug-Reports aber unerlässlich. Ich schreibe keine Log Files mit Fehlern, daher ist diese Variante sehr viel leichter zu implementieren und man kann den Fehler auch sehr gut nachvollziehen.

Du versteht wieder das Problem nicht. Wieso stdout? Eine Bibliothek hat niemals etwas auf stdout auszugeben (außer die stdio-Bibliothek), denn da gehören Ausgaben des Anwendungscodes hin. Wozu sind denn deiner Meinungnach stderr und stdlog da, wenn nicht zur Dokumentation vor Error- und Logdaten?

Ich gestehe, ich habe stderr und stdlog noch nie aktiv verwendet. Führe ich mir zu Gemüte und werde den Code entsprechend ändern, dort Fehler protokollieren zu lassen.

- Kandidaten für Überläufe sind vorprogrammiert (im wahrsten Sinn):
char attributeBuffer[1024];
...
  char xRef[64];
sowas macht man mit symbol. Konstanten und verwendet diese Größenangaben natürlich dann später wieder
Nein, weil, wenn du schaust, der Buffer wird beim erreichen der Grenze von 1024 Bytes geleert bzw. auf 0 Position gesetzt. Es kann also daher aktuell kein Buffer Overflow auftreten. Es kann aber auch gut sein das ich dies später hinzugefügt habe.

Du verstehst schon wieder das Problem nicht. Du musst an jeder Stelle im Programm genau wissen, dass dieses Array 1024 groß ist. Wenn es sich jemals ändert, musst du genau alle Stellen im Programm ändern, wo es benutzt wird. Eine Stelle vergessen und schon kracht es.

Ich habe das Problem verstanden, ich hatte bereits geschrieben dass mir der Sinn bzw. der Nutzen (auch @hustenbaer) einer eigens definierten Konstante als "Puffergröße" sehr wohl einleuchtet. Ich streite auch nicht ab dass dies generell die Wartbarkeit erhöht ohne alles im Code zu überfliegen wo man im Gegenzug dazu auf ein fix eingegebene Zahl hin überprüft (hatte).

int readUntil = atol(xRef);
unsauber und fehlerintolerant (atol liefert long und nicht int, atol liefert 0 bei Fehlern,...)
Mit dem Long stimmt, so sollte dies auch sein - selbst wenn der Wert 0 ist, wird eine Alternative angeboten wenn nichts bekannt ist.
Was wäre deiner Meinung nach die beste Alternative [...]

Du verstehst schon wieder das Problem nicht. Du machst keine vernünftige Fehlerbehandlung. Du kannst gar keine vernünftige Fehlerbehandlung machen, da atol dies gar nicht zu lässt. Woher willst du wissen, ob ein Wert ungültig war oder ob es eine echte 0 war? Nimm die richtigen Funktionen für diese Aufgabe, also die strtol & co.[/quote]

In dem Punkt stimme ich dann aber wieder mit dir überein und danke für den Hinweis auf strtol. Das werde ich auch übernehmen.

Du scheinst insgesamt eher da dran interessiert, deinen schlechten Code zu verteidigen, als dich ernsthaft mit der Kritik auseinander zu setzen. Wutz hat das nicht geschrieben, um dich persönlich anzugreifen, sondern um zu zeigen, was an dem Code falsch/schlecht ist. Vielleicht fehlt dir auch einfach das Wissen, um seine Kritik richtig zu verstehen.

Nein, es ist keine Verteidigung von schlechtem Code oder liegt daran das ich stur wäre, ganz im Gegenteil.
Ich versuche euch nur verständlich zu machen welche Beweggründe ich habe oder hatte bzw. wo mein aktuelles Verständnis liegt. Wenn ich Sachen nicht perfekt produziert habe, bin ich grundsätzlich auch für Kritik empfänglich. Aber dann verlange ich auch Gründe bzw. Alternativen wie man es denn richtig macht, damit ich daraus lernen kann.

Mechanics

theSplit schrieb:

Ich habe nur leider auch festgestellt das diverse Aufrufe zu "gtk_main_iteration()" dazu führen, das die Leseroutine massiv ausgebremst wird - jedenfalls ohne eine Art von Threads zu verwenden.

Threads zu verwenden ist die übliche Vorgehensweise und gehört auch in den Anwendungscode, nicht in die Bibliothek.

theSplit

Mechanics schrieb:

theSplit schrieb:

Ich habe nur leider auch festgestellt das diverse Aufrufe zu "gtk_main_iteration()" dazu führen, das die Leseroutine massiv ausgebremst wird - jedenfalls ohne eine Art von Threads zu verwenden.

Threads zu verwenden ist die übliche Vorgehensweise und gehört auch in den Anwendungscode, nicht in die Bibliothek.

Hi,

ja, das hätte ich auch so vorgehabt - dass das Frontend die Bibliothekroutinen als Thread aufruft. Ich weiß nur noch nicht wann ich das implementieren kann.

---

Im übrigen prüfe ich nun, ich hoffe so ist es korrekt, in dieser Form nach EOF (gegen einen Int wert):

char currentChar = '\0';
int charVal = 0;

while ((charVal = fgetc(inputFile)) != EOF) {
  currentChar = (char) charVal;
}

Die anderen Anmerkungen sind bereits eingebaut. Also strtol statt atol, auch mit einem long int und nicht fälschlicherweise in einem Int gespeichert. Ein "#define" für den Pufferwert, EOF Prüfung. "rb" statt "r" Modus beim Öffnen von Dateien.

Über SEEK_END und ftell bin ich allerdings noch unschlüssig wie das anders gehen, sollte.

SeppJ

So kann man korrekt nach EOF prüfen (den Cast kannst du dir übrigens sparen). Merke: EOF ist, nachdem versucht wurde, über das Ende einer Datei zu lesen.