Ok, dann geht die Kritik mal weiter
Bin gerade mal durch deinen Code gegangen (hauptsächlich den der Liste) und dabei sind mir ein paar Dinge aufgefallen. Als erstes natürlich die schon angesprochenen Layout-Probleme. Der Code ist echt an vielen Stellen schwer lesbar und ich glaube das macht es leichter für Fehler.
Zudem wirkt der Code nicht sehr einheitlich, einige male werden Worte große andere male klein geschrieben, mal Dinge geprüft, mal nicht. Ich würde mal sagen der Code stammt nicht von einer Person, oder? Schätze mal das sind kleine Stücken von woanders mit eingeflossen, aber ist ja auch ok, falls dem nicht so ist, so wirkt es halt nur. Ist so oder so vollkommen ok.
Im Allgemeinen möchte ich erstmal sagen, dass der Code natürlich nicht perfekt ist (wessen ist das schon?!) und man könnte einige Dinge eleganter lösen, aber rein semantisch sah er jetzt auch nicht falsch aus. Und das wichtigste ist, er muss funktionieren (denke mal das tut er!).
Was mich ein wenig wundert ist immer noch die Tatsache das du zwei Listen verwendest. Eine für Autoren, eine für Bücher. Die enthalten dummerweise die gleichen Daten, von daher sollte man auch wirklich nur eine Liste verwenden. Schöner wäre es wenn du zwei verschiedene Suchkriterien in einer Liste hast. So dass du in der Liste immer auf die selben Daten zugreifst und einmal halt den Titel und ein anderes mal den Autor vergleichst. Assymptotisch macht das wenig unterschied was Platzbedarf angeht, aber real brauchst du so doppelt so viel Speicherplatz, unnötig.
Ja, wenn du ein Objekt aus der Liste entfernst, sehe ich dass du ganz sauber die Zeiger auf dieses Element entfernst, aber in Delphi solltest du dieses Objekt auch freigeben. Dazu dient der Aufruf des Destruktors (.Free)
Etwas ungewöhlich ist auch deine Art über die Liste zu laufen. Du gibst nie ein Element direkt zurück, sondern machst alles über den zeiger Position (oder so). Wie gesagt, nur ungewöhnlich (wunderte mich zum Beispiel bei isElement, dass da der Zeiger neu gesetzt wird).
Dann solltest du nicht direkt auf FNext und FPrevious zugreifen, wenn die privat sind und der Zugriff von aussen kommt. Besser die beiden public machen oder Setter/Getter verwenden (schau mal hier nach Properties). Sichtbarkeit sollte halt schon beachtet werden, auch wenn Delphi private Felder in der gleichen Datei anzeigt.
Ja, deine verzweigten if - Anweisungen kannst du etwas vereinfachen und zusammen fassen
Delphi-Quellcode:
// statt
if a then
begin
end
else
if b then
begin
end;
else
begin
end;
// lieber
if a then
begin
end
else if b then
begin
end
else
begin
end;
Total dass gleiche aber leichter lesbar.
Wenn man bei dir Next oder Previous aufruft und die Liste nur ein Element hat, würde ich den Nullzeiger (nil) erwarten, denke bei dir wäre es aber das eine Element, ist dass so beabsichtigt?
Und ganz wichtig für die Arbeit mit FileStreams, das Create sollte immer in einen Try ... finally Block, da schon mit dem Create versucht wird die Datei zu öffnen. Kommt es hier zu einem Fehler, sollte das Objekt trotzdem freigegeben werden.
So ganz allgemein noch, du solltest nicht all zu große Prozeduren schreiben. Viele kleine machen vieles leichter (häufig). Natürlich nicht nur einzeiler, aber ruhig Dinge die zusammengehören mal auslagern (in ne eigene Prozedur).
Dann sind Case Anweisungen mit true und false eigentlich schöner mit einem if realisierbar. Und zu guter letzt, wenn du etwas in jedem Fall machst
Delphi-Quellcode:
if a then
begin
...
doFoo;
end
else
begin
...
doFoo;
end;
Dann schreib das aus den Bedingungen raus (weniger Fehler beim Erweitern und weniger Schreibarbeit)
Delphi-Quellcode:
if a then
begin
...
end
else
begin
...
end;
doFoo;
Ja, ich hab mal ein wenig eingerückt, damit du siehst was ich meine, wie es alternativ aussehen könnte. Bleibt nur ein Tipp. An sich ist es wie gesagt wichtig fehlerfreien (und damit funktionierenden) Code zu erzeugen. Dein Code ist also nicht schlecht nur im Moment noch schwer lesbar (haben wir alle mal gehabt). An sich sind aber wie gesagt keine Fehler drin die mir aufgefallen wären, also nicht schlecht.
Gruß Der Unwissende