|
Antwort |
Registriert seit: 7. Okt 2004 Ort: Solingen 11 Beiträge |
#1
Hallo
Ich habe in der Schule die Aufgabe erhalten eine Bücher.txt mit 2000 Datensätzen in eine Liste zu schreiben und diese Liste nach Autoren bzw. Titel zu durchlaufen und zu sortieren. Ich habe es geschrieben, aber ich weiß nicht, ob es so wirklich "gute geschrieben" ist. Könntet ihr euch das einmal anschauen? Ich hänge den Code an:
Delphi-Quellcode:
unit mSortListe;
interface uses Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms, Dialogs, StdCtrls, mDListe, XPMan, Buttons; const max=2000; type TBuch = class(TSortElement) Sachgebiet, Autor, Titel, Ort, Verlag : String; Jahr : integer; function gleich (zweites:TSortElement):boolean;override; function kleiner (zweites:TSortElement):boolean;override; end; type TAutor = class(TSortElement) Sachgebiet, Autor, Titel, Ort, Verlag : String; Jahr : integer; function gleich (zweites:TSortElement):boolean;override; function kleiner (zweites:TSortElement):boolean;override; end; type TfMain = class(TForm) Memo1: TMemo; lbAnzahl: TLabel; edTitel: TEdit; edAutor: TEdit; edSachgebiet: TEdit; btNext: TButton; btPrevious: TButton; edVerlag: TEdit; edJahr: TEdit; btlast: TButton; btfirst: TButton; edOrt: TEdit; rbautor: TRadioButton; rbtitel: TRadioButton; Label1: TLabel; Label2: TLabel; Label3: TLabel; Label4: TLabel; Label5: TLabel; Label6: TLabel; Label7: TLabel; edSuche: TEdit; btSuche: TButton; XPManifest1: TXPManifest; BitBtn1: TBitBtn; procedure FormCreate(Sender: TObject); procedure Anzeigen; procedure btNextClick(Sender: TObject); procedure btPreviousClick(Sender: TObject); procedure btfirstClick(Sender: TObject); procedure btlastClick(Sender: TObject); procedure rbautorClick(Sender: TObject); procedure rbtitelClick(Sender: TObject); procedure btSucheClick(Sender: TObject); procedure BitBtn1Click(Sender: TObject); private Feld : array [0..max] of TBuch; Anzahl:Integer; TListe,AListe : TDListe; end; var fMain: TfMain; implementation {$R *.dfm} function TBuch.gleich (zweites:TSortElement):boolean; begin gleich := Self.Titel = TBuch(zweites).Titel end; function TBuch.kleiner (zweites:TSortElement):boolean; begin kleiner := Self.Titel < TBuch(zweites).Titel end; function TAutor.gleich (zweites:TSortElement):boolean; begin gleich := Self.Autor = TAutor(zweites).Autor end; function TAutor.kleiner (zweites:TSortElement):boolean; begin kleiner := Self.Autor < TAutor(zweites).Autor end; procedure TfMain.FormCreate(Sender: TObject); var i, Code:integer; Zeile : String; neu : TBuch; neu2 :TAutor; begin memo1.Lines.LoadFromFile('Bücher.txt'); Anzahl := memo1.Lines.Count; TListe := TDListe.Create; AListe := TDListe.Create; for i := 0 to Anzahl-1 do begin neu := TBuch.Create; with neu do begin Zeile := Memo1.Lines[i]; delete (zeile, 1, pos(';', zeile)); sachgebiet := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); Autor := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); Titel := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); Ort := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); if copy(Zeile, 1, pos(';', zeile)-1) = '' then Jahr :=0 else val(copy(Zeile, 1, pos(';', zeile)-1),Jahr,Code ); delete (zeile, 1, pos(';', zeile)); Verlag := Zeile; end; TListe.Insert(neu); end; memo1.Lines.LoadFromFile('Bücher.txt'); i:=0; TListe.First; for i := 0 to Anzahl-1 do begin neu2 := TAutor.Create; with neu2 do begin Zeile := Memo1.Lines[i]; delete (zeile, 1, pos(';', zeile)); sachgebiet := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); Autor := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); Titel := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); Ort := copy(Zeile, 1, pos(';', zeile)-1); delete (zeile, 1, pos(';', zeile)); if copy(Zeile, 1, pos(';', zeile)-1) = '' then Jahr :=0 else val(copy(Zeile, 1, pos(';', zeile)-1),Jahr,Code ); delete (zeile, 1, pos(';', zeile)); Verlag := Zeile; end; AListe.Insert(neu2); end; AListe.First; lbAnzahl.Caption := intToStr(Anzahl); end; procedure TfMain.Anzeigen; var buch :TBuch; buch2:TAutor; begin buch := TBuch(TListe.GetElement); buch2 := TAutor(AListe.GetElement); case fMain.rbtitel.Checked of true : begin with buch do begin edTitel.Text := Titel; edAutor.Text := Autor; edSachgebiet.Text := Sachgebiet; edVerlag.Text := Verlag; edJahr.Text:= inttostr(Jahr); edOrt.Text := Ort; end; end; false : begin with buch2 do begin edTitel.Text := Titel; edAutor.Text := Autor; edSachgebiet.Text := Sachgebiet; edVerlag.Text := Verlag; edJahr.Text:= inttostr(Jahr); edOrt.Text := Ort; end; end; end; end; procedure TfMain.btNextClick(Sender: TObject); var buch :TBuch; begin case fMain.rbtitel.Checked of true : begin TListe.Next; Anzeigen; end; false: begin AListe.Next;Anzeigen; end; end; end; procedure TfMain.btPreviousClick(Sender: TObject); var buch :TBuch; begin case fMain.rbtitel.Checked of true : begin TListe.Previous; Anzeigen; end; false: begin AListe.Previous; Anzeigen; end; end; end; procedure TfMain.btfirstClick(Sender: TObject); begin case fMain.rbtitel.Checked of true : begin TListe.First; Anzeigen; end; false: begin AListe.First; Anzeigen; end; end; end; procedure TfMain.btlastClick(Sender: TObject); begin case fMain.rbtitel.Checked of true : begin TListe.Last; Anzeigen; end; false: begin AListe.last; Anzeigen; end; end; end; procedure TfMain.rbautorClick(Sender: TObject); var buch :TBuch; begin buch := TBuch(TListe.GetElement); AListe.First; while (not Aliste.isLast) and (buch.Autor <> TBuch(AListe.GetElement).Autor) do begin AListe.Next; end; // Anzeigen; end; procedure TfMain.rbtitelClick(Sender: TObject); var buch :TAutor; begin buch := TAutor(AListe.GetElement); TListe.First; while not Tliste.isLast and (buch.Autor <> TBuch(TListe.GetElement).Autor) do begin tListe.Next; end; Anzeigen; end; procedure TfMain.btSucheClick(Sender: TObject); var gefunden:boolean; i,p,z:integer; begin gefunden := false; z:= 1; case fMain.rbtitel.Checked of true : begin TListe.First; while not Tliste.isLast do begin z:= z+1; i := Pos(LowerCase(fMain.edSuche.Text), LowerCase(TBuch(TListe.GetElement).titel)); if i <> 0 then begin gefunden:=true; break; end else tliste.next; end; if gefunden then Anzeigen else showmessage(inttostr(i) + 'Nicht gefunden'); end; false: begin AListe.First; while not Aliste.isLast do begin z:= z+1; i := Pos(LowerCase(fMain.edSuche.Text), LowerCase(TAutor(AListe.GetElement).titel)); if i <> 0 then begin gefunden:=true; break; end else tliste.next; end; if gefunden then Anzeigen else showmessage(inttostr(i) + 'Nicht gefunden'); end; end; end; procedure TfMain.BitBtn1Click(Sender: TObject); begin Case MessageDlg ('Titelliste speichern?', mtConfirmation, [mbAbort, mbYes, mbNo], 0) of mrYes: TListe.ComponentSaveToFile(self,'Sortiert.txt'); // Hier der Code bei Yes mrNo: AListe.ComponentSaveToFile(self,'Sortiert_autor.txt') ; // Hier der Code bei No mrCancel: ; // Hier der Code bei Cancel End; showmessage('toll'); end; end.
Delphi-Quellcode:
unit mDListe;
interface uses classes,Sysutils; type TSortElement = class (TObject) private FNext, FPrevious : TSortElement; public constructor Create; function gleich (zweites:TSortElement):boolean;virtual;abstract; function kleiner (zweites:TSortElement):boolean;virtual;abstract; end; TDListe = class(TObject) private FRoot, FBottom, FPosition : TSortElement; public constructor Create; function GetElement: TSortElement; function isElement(Elem:TSortElement):boolean; procedure Insert (Elem:TSortElement); procedure ComponentSaveToFile(Component: TComponent; const FileName: String); procedure Delete; Procedure Next; procedure First; Procedure Previous; procedure Last; function isEmpty : Boolean; function isLast : Boolean; function isFirst : Boolean; End; implementation constructor TSortElement.Create; begin inherited create; FNext := nil; FPrevious :=nil; end; constructor TDListe.Create; begin inherited create; FRoot:=nil; FPosition:=FRoot; FBottom:=nil; end; function TDListe.GetElement: TSortElement; begin GetElement:=FPosition; end; function TDListe.isElement(Elem:TSortElement):boolean; var gefunden : boolean; hilf : TSortElement; begin gefunden := false; hilf := FRoot; if not isEmpty then begin while (hilf <> FBottom) and hilf.kleiner(Elem) do hilf := hilf.FNext; gefunden := FPosition.gleich(Elem); end; if gefunden then FPosition := hilf; isElement := gefunden; end; procedure TDListe.Insert (Elem:TSortElement); var hilf : TSortElement; begin if isEmpty then begin FRoot := Elem; FPosition := Elem; FBottom := Elem; end else begin hilf := FRoot; while (hilf.FNext <> nil) and (hilf.kleiner(Elem)) do hilf := hilf.FNext; if hilf = FRoot then begin FRoot := Elem; FRoot.Fnext := hilf; hilf.FPrevious := FRoot end else if (hilf.FNext = nil) and hilf.kleiner(Elem) then begin hilf.FNext:=Elem; Elem.FPrevious:=hilf; FBottom:=Elem; end else begin hilf.FPrevious.FNext:=Elem; Elem.FNext:= hilf; Elem.FPrevious:=hilf.FPrevious;hilf.FPrevious:=Elem; end; end; end; procedure TDListe.Delete; begin if FRoot = FBottom then begin FRoot := nil; FBottom := nil; end else if FPosition = FRoot then begin FRoot := FPosition.FNext; FRoot.FPrevious := nil; end else if FPosition = FBottom then begin FBottom := FBottom.FPrevious; FBottom.FNext := nil; end else begin FPosition.FNext.FPrevious := FPosition.FPrevious; FPosition.FPrevious.FNext := FPosition.FNext; end; end; Procedure TDListe.Next; begin if FPosition <> nil then if FPosition.FNext <> nil then FPosition:=FPosition.FNext; end; procedure TDListe.First; begin FPosition := FRoot; end; Procedure TDListe.Previous; begin if FPosition <> nil then if FPosition.FPrevious <> nil then FPosition:=FPosition.FPrevious; end; procedure TDListe.Last; begin FPosition := FBottom; end; function TDListe.isEmpty : Boolean; begin isEmpty := FRoot=nil end; function TDListe.isLast : Boolean; begin isLast := FPosition=FBottom end; function TDListe.isFirst : Boolean; begin isFirst := FPosition=FRoot end; procedure TDliste.ComponentSaveToFile(Component: TComponent; const FileName: String); var fs: TFileStream; begin fs := TFileStream.Create(FileName, fmCreate); try fs.WriteComponentRes(Component.Name, Component); finally fs.Free; end; end; end. Vielen Dank im voraus! MfG |
Zitat |
Registriert seit: 13. Dez 2003 Ort: Berlin 1.756 Beiträge |
#2
Zitat von Mr_Anderson:
Hallo
Ich habe in der Schule die Aufgabe erhalten eine Bücher.txt mit 2000 Datensätzen in eine Liste zu schreiben und diese Liste nach Autoren bzw. Titel zu durchlaufen und zu sortieren. Ich habe es geschrieben, aber ich weiß nicht, ob es so wirklich "gute geschrieben" ist. Könntet ihr euch das einmal anschauen? Ich hänge den Code an: erstmal zum Thema Schulaufgaben und DP, es ist eine der Grundregeln, dass Schulaufgaben nicht komplett durch Mitglieder der DP gelöst werden. Keine Sorge, ich seh schon ein dass du hier deine Lösung postest und nicht nach einer Lösung fragst, aber ich wollte es schon mal gesagt haben. Auf jeden Fall hast du keinen Code angehangen (dazu gibt es in dem Fenster wo du auch deinen Beitrag schreibst weiter unten einige Felder) und ich glaube solange du die Datei einfach nur postest (und nicht anhängst) wird sich keiner deinen Code anschauen. Ist einfach zu mühsam im Browser. Also bitte anhängen, danke. Ansonsten wäre es natürlich schöner, wenn du etwas genauer sein könntest. Wenn du dir an einer Stelle (oder halt an mehreren) nicht sicher bist, benenn einfach die Stelle, den ganzen Code durchgehen ist halt ein wenig so, als ob du Arbeit abschieben möchtest. Nicht falsch verstehen! Aber ein paar Dinge, die mir sofort auffallen. Es fängt eigentlich damit an, dass du sehr wenig Struktur in deinem Code hast. Es gehört ein wenig zur gängigen Konvention dass man kleinere Regeln einhält. D.h. du musst dich nicht dran halten, aber du solltest es so gut wie möglich. Man sollte es sich vorallem immer früh angewöhnen, je später desto schwerer. Wie gesagt, keine böse Kritik, nur Tipps. Für Kritik wär ich eh der falsche, ich halte mich weniger an die Delphi-Konvention als vielmehr an welche die ich hauptsächlich aus Java (und ein paar anderen Sprachen) kenne. Also, mal der erste Block
Delphi-Quellcode:
Hier gäbe es gleich mehrere Dinge die ich persönlich bemängeln würde. Du solltest auf jeden Fall mehr Leerzeilen verwenden. Da sieht man schneller wo was aufhört und was neues anfängt.
unit mSortListe;
interface uses Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms, Dialogs, StdCtrls, mDListe, XPMan, Buttons; const max=2000; type TBuch = class(TSortElement) Sachgebiet, Autor, Titel, Ort, Verlag : String; Jahr : integer; function gleich (zweites:TSortElement):boolean;override; function kleiner (zweites:TSortElement):boolean;override; end; type TAutor = class(TSortElement) Sachgebiet, Autor, Titel, Ort, Verlag : String; Jahr : integer; function gleich (zweites:TSortElement):boolean;override; function kleiner (zweites:TSortElement):boolean;override; end; Konstanten sollte man möglichst immer GROß schreiben (sorry, will nicht schreien, sondern meine nur alle Buchstaben groß). Type brauchst du nicht vor jeden Block schreiben. Und bei deiner Objekt-Hierachie wäre ich mir auch nicht so sicher (hat ein Autor denn einen Autor, einen Titel und einen Ort? Beim Rest könnte man drüber streiten). Also mal rein zur Formatierung versuch es mal so :
Delphi-Quellcode:
Hoffe du siehst ein wenig was ich meine. Und wo kommt eigentlich die Klasse TSortElement her?
unit mSortListe;
interface uses Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms, Dialogs, StdCtrls, mDListe, XPMan, Buttons; CONST MAX = 2000; type TBuch = class(TSortElement) public Sachgebiet, Autor, Titel, Ort, Verlag : String; Jahr : Integer; function gleich(zweites:TSortElement) : Boolean; override; function kleiner(zweites:TSortElement) : Boolean; override; end; TAutor = class(TSortElement) Sachgebiet, Autor, Titel, Ort, Verlag : String; Jahr : Integer; function gleich (zweites:TSortElement) : Boolean; override; function kleiner (zweites:TSortElement) : Boolean; override; end; Eine weitere Sache ist es, möglichst Dinge die zusammen gehören auch in eine Datei zu schreiben statt alles in einer unter zu bringen. Auch dass schafft etwas mehr Übersicht. Zwischen zwei Methoden solltest du auch mindestens eine Leerzeile lassen und
Delphi-Quellcode:
sieht auch nicht hübsch aus. Lieber etwas offensichtlicher die Hierachie anzeigen
function TBuch.gleich (zweites:TSortElement):boolean;
begin gleich := Self.Titel = TBuch(zweites).Titel end;
Delphi-Quellcode:
Gut, du solltest natürlich auch die Typsicherheit (if zweites is TBuch then ...) prüfen, aber ich glaube das kommt schon noch (später).
function TBuch.gleich(zweites : TSortElement) : Boolean;
begin gleich := Self.Titel = TBuch(zweites).Titel end; Ja, wie gesagt nich böse sein oder so, nur tipps und die Datei anhängen sonst liest es keiner Gruß Der Unwissende |
Zitat |
Registriert seit: 7. Okt 2004 Ort: Solingen 11 Beiträge |
#3
hi
Danke für die Kritik, habe versucht diese direkt einmal umzusetzen Anhang: Das Projekt
"Ich mag Schweine. Hunde schauen zu uns auf, Katzen schauen auf uns herab - nur Schweine betrachten uns als ihresgleichen." (Winston Churchill)
|
Zitat |
Registriert seit: 12. Dez 2004 Ort: Wien, Österriech 893 Beiträge Delphi 6 Enterprise |
#4
Wenn wir schon bie der Formatierung sind, dann würde ich es so machen :
Delphi-Quellcode:
type
TBuch = class(TSortElement) public Sachgebiet : String; Autor : String; Titel : String; Ort : String; Verlag : String; Jahr : Integer; function gleich(zweites:TSortElement) : Boolean; override; function kleiner(zweites:TSortElement) : Boolean; override; end;
Katura Haris
Es (ein gutes Wort) ist wie ein guter Baum, dessen Wurzel fest ist und dessen Zweige in den Himmel reichen. |
Zitat |
Registriert seit: 13. Dez 2003 Ort: Berlin 1.756 Beiträge |
#5
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:
Total dass gleiche aber leichter lesbar.
// 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; 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:
Dann schreib das aus den Bedingungen raus (weniger Fehler beim Erweitern und weniger Schreibarbeit)
if a then
begin ... doFoo; end else begin ... doFoo; end;
Delphi-Quellcode:
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.
if a then
begin ... end else begin ... end; doFoo; Gruß Der Unwissende |
Zitat |
Ansicht |
Linear-Darstellung |
Zur Hybrid-Darstellung wechseln |
Zur Baum-Darstellung wechseln |
ForumregelnEs ist dir nicht erlaubt, neue Themen zu verfassen.
Es ist dir nicht erlaubt, auf Beiträge zu antworten.
Es ist dir nicht erlaubt, Anhänge hochzuladen.
Es ist dir nicht erlaubt, deine Beiträge zu bearbeiten.
BB-Code ist an.
Smileys sind an.
[IMG] Code ist an.
HTML-Code ist aus. Trackbacks are an
Pingbacks are an
Refbacks are aus
|
|
Nützliche Links |
Heutige Beiträge |
Sitemap |
Suchen |
Code-Library |
Wer ist online |
Alle Foren als gelesen markieren |
Gehe zu... |
LinkBack |
LinkBack URL |
About LinkBacks |