Problem bei TStringList

[muffin104k6]

Hallo,
ich habe bei meiner Anwendung folgendes Problem:
Mithilfe einer Stringlist werden Uhrzeiten mit Informationen eingelesen.
Wenn die eingegebene Uhrzeit erreicht wurde, soll der Eintrag gelöscht werden. Allerdings werden Einträge gelöscht, die noch gar nicht erreicht wurden.
Die Codes:
Daten speichern bzw. ausgeben:

Delphi-Quellcode:

procedure PrintOutAuto;
var
  Zeit: String;
  Info: String;
  StringRoh: String;
  Zeilen: Integer;
begin
  Zeilen:=slA.Count; //Füllen der Zeilen
  ShowMessage(IntTOStr(Zeilen));
  if slA.Text <> '' then
  if Zeilen >= 1 then
  //if Length(slA[0]) >= 1 then
   begin
   ShowMessage('Z1,slA[0]');
   StringRoh:=slA[0];
   Zeit:=(Copy(StringRoh,1,5));
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Form1.Show1.Caption:=(Zeit + ' - ' + Info);
   end
  else
  begin
   Form1.Show1.Visible:=false;
   Form1.Show2.Visible:=false;
   Form1.Show3.Visible:=false;
   Form1.Show4.Visible:=false;
   Form1.Show5.Visible:=false;
   end;
  if Zeilen >= 2 then
  //if Length(slA[1]) >= 1 then
   begin
   ShowMessage('Z2,slA[1]');
   StringRoh:=slA[1];
   Zeit:=(Copy(StringRoh,1,5));
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Form1.Show2.Caption:=(Zeit + ' - ' + Info);
   end
  else
  begin
   Form1.Show2.Visible:=false;
   Form1.Show3.Visible:=false;
   Form1.Show4.Visible:=false;
   Form1.Show5.Visible:=false;
   end;
  if Zeilen >= 3 then
  //if Length(slA[2]) >= 1 then
   begin
   ShowMessage('Z3,slA[2]');
   StringRoh:=slA[2];
   Zeit:=(Copy(StringRoh,1,5));
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Form1.Show3.Caption:=(Zeit + ' - ' + Info);
   end
  else
  begin
   Form1.Show3.Visible:=false;
   Form1.Show4.Visible:=false;
   Form1.Show5.Visible:=false;
   end;
  if Zeilen >= 4 then
  //if Length(slA[3]) >= 1 then
   begin
   ShowMessage('Z4,slA[3]');
   StringRoh:=slA[3];
   Zeit:=(Copy(StringRoh,1,5));
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Form1.Show4.Caption:=(Zeit + ' - ' + Info);
   end
  else
  begin
   Form1.Show4.Visible:=false;
   Form1.Show5.Visible:=false;
   end;
  if Zeilen >= 5 then
  //if Length(slA[4]) >= 1 then
   begin
   ShowMessage('Z5,slA[4]');
   StringRoh:=slA[4];
   Zeit:=(Copy(StringRoh,1,5));
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Form1.Show5.Caption:=(Zeit + ' - ' + Info);
   end
  else
  begin
   Form1.Show5.Visible:=false;
   end;
   {für die ersten 5: zeit + info ausgeben}
 if Form1.Show1.Visible=false then    //Abfangen, falls ShowLabel nicht sichtbar ist
  begin
 if slA.Count >= 1 then               //Nur aktivieren, wenn wirklich benötigt
  begin
   Form1.Show1.Visible:=true;
  end;
  end;
 if Form1.Show2.Visible=false then
  begin
 if slA.Count >= 2 then
  begin
  Form1.Show2.Visible:=true;
  end;
  end;
 if Form1.Show3.Visible=false then
  begin
 if slA.Count >= 3 then
  begin
   Form1.Show3.Visible:=true;
  end;
  end;
 if Form1.Show4.Visible=false then
  begin
 if slA.Count >= 4 then
  begin
   Form1.Show4.Visible:=true;
  end;
  end;
 if Form1.Show5.Visible=false then
  begin
 if slA.Count >= 5 then
  begin
   Form1.Show5.Visible:=true;
  end;
  end;

end;

Nach Ablauf checken:

Delphi-Quellcode:

procedure TForm2.Button3Click(Sender: TObject);
var
  Zeilen: Integer;
  Zeile: Integer=0;
  StringRoh: String;
  Zeit: TTime;
  Info: String;
  fs: TFileStream;
  LB: String;
begin
 Zeilen:=slA.Count;
  while Zeilen > 0 do
   begin                                                     //-> PrintOutAuto
   StringRoh:=slA[Zeile];
   Zeit:=StrToTime(Copy(StringRoh,1,5));
   Info:=StringRoh;
   Delete(Info,1,6);
   //ShowMessage(TimeToStr(Zeit));
   //ShowMessage(Info);
   If Now > Zeit then {Hier liegt wohl das Problem}
    begin
     slA.Delete(Zeile);
     slA.Text:=Trim(slA.Text);
     //slA.SaveToFile('slA.log');
     Zeilen:=slA.Count;
     PrintOutAuto;
    end;
   Zeile:=Zeile+1;
   Zeilen:=slA.Count-1;
   end;
end;

Ich hoffe Ihr steigt da durch.

[himitsu]

Warum ist PrintOutAuto keine Methode der TForm1? (immerhin wird da drin ständig auf Form1 zugegriffen)

Vergleiche mit

Delphi-Quellcode:

if x=True then

und

Delphi-Quellcode:

if x=False then

sollte man besser nicht machen. Abgesehn davon sind das ja bereits Boolean,
also reicht ein

Delphi-Quellcode:

if x then

oder

Delphi-Quellcode:

if not x then

. Und ja, das kann auch Fehler verursachen, denn ein Boolean ist nunmal nicht nur True und False, sondern es gibt (theoretisch) noch 254 andere Werte, welche als True (eigentlich als "nicht False") gelten und die erwischt man mit

Delphi-Quellcode:

=True

nunmal nicht.

Dank der kaum vorhandenen Codeformatierung erkenn ich eh grad nicht, was da überhaupt passieren soll. (vorallem in dem PrintOutAuto)
Es sieht aber auch noch irgendwie so aus, als bestehe diese PrintOutAuto-Methode eh zu 80% aus doppeltem Code, welcher sich bestimmt zusammenfassen läßt, um die Lesbarkeit zu erhöhen.

Zitat:

Delphi-Quellcode:

{Hier liegt wohl das Problem}

Keine Ahnung, aber was sagt denn dein Debugger, wenn grade ein Eintrag gelöscht wird? Stimmen denn die Werte damit überein, daß der Eintrag wirklich gelöscht werden muß?

Wenn man Einträge löscht, dann entweder die Liste rückwärts durchgehen, da sonst die Einträge nach dem gelöschten Eintrag übersprungen werden,
odr du darfst in diesem Fall den Index (Zeile) nicht hochzählen.

[HeZa]

Zitat:

If Now > Zeit then {Hier liegt wohl das Problem}

Alles ungetestet dahin gesagt
Now enthält immer auch das Datum und sollte somit immer größer als TTime Wert sein.

Beispiel
12:00 entspricht 0,5
heute 12:00 (Now um 12:00) entspricht 46234,5 (Ok, die 46234 habe ich mir jetzt aus gedacht weil ich keine IDE offen habe, :-) aber irgendetwas in diesem Bereich wird es sein)

Ciao Heinz Z.

[himitsu]

Ahhh, da kommt die "Zeit" her,

Joar, das ist schonmal ein Grund, wobei das der Debugger natürlich sofort zeigen würde, wenn man Diesen benutzt und sich beide Werte ansieht.
Time, TimeOf oder Dergleichen.

Und warum dabei nicht alles, sondern nur die Hälfte gelöscht wird, hatte ich auch schon erklärt.

[Popov]

@muffin104k6

Ich will dir nicht vorwerfen, dass du deinen Code nicht strukturiert formatiert hast, aber bei dem zweiten If habe ich bereits die Übersicht verloren. Ich empfehle dir da etwas mehr Luft drin, dann erkennt man auch schneller die Zusammenhänge.

ist es richtig, dass hier

Delphi-Quellcode:

  if slA.Text <> '' then
  if Zeilen >= 1 then

das erste If nicht für alles gilt, sondern nur für die erste Abfrage? Kann es ein, dass du da ein Block vergessen hast?

[himitsu]

Jup, im Prinzip könnte man denken daß dort ein Begin-End fehlt, aber letztendlich ist das egal, da beide Vergleiche genau das selbe machen.
Und da die nächsten IFs darauf aufbauen (noch mehr Zeilen), kürzt sich das erste IF so oder so logisch gesehn eh weg.

Delphi-Quellcode:

procedure TForm1.PrintOutAuto;
var
  Zeit: String;
  Info: String;
  StringRoh: String;
  Zeilen: Integer;
begin
  Zeilen:=slA.Count; //Füllen der Zeilen
  ShowMessage(IntTOStr(Zeilen));
  if Zeilen >= 1 then
  //if Length(slA[0]) >= 1 then
   begin
   ShowMessage('Z1,slA[0]');
   StringRoh:=slA[0];
   Zeit:=Copy(StringRoh,1,5);
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Show1.Caption:=Zeit + ' - ' + Info;
   end;
  if Zeilen >= 2 then
  //if Length(slA[1]) >= 1 then
   begin
   ShowMessage('Z2,slA[1]');
   StringRoh:=slA[1];
   Zeit:=Copy(StringRoh,1,5);
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Show2.Caption:=Zeit + ' - ' + Info;
   end;
  if Zeilen >= 3 then
  //if Length(slA[2]) >= 1 then
   begin
   ShowMessage('Z3,slA[2]');
   StringRoh:=slA[2];
   Zeit:=Copy(StringRoh,1,5);
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Show3.Caption:=Zeit + ' - ' + Info;
   end;
  if Zeilen >= 4 then
  //if Length(slA[3]) >= 1 then
   begin
   ShowMessage('Z4,slA[3]');
   StringRoh:=slA[3];
   Zeit:=Copy(StringRoh,1,5);
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Show4.Caption:=Zeit + ' - ' + Info;
   end;
  if Zeilen >= 5 then
  //if Length(slA[4]) >= 1 then
   begin
   ShowMessage('Z5,slA[4]');
   StringRoh:=slA[4];
   Zeit:=Copy(StringRoh,1,5);
   Info:=StringRoh;
   if Length(StringRoh) >6 then Delete(Info,1,6);
   Show5.Caption:=Zeit + ' - ' + Info;
   end;
  {für die ersten 5: zeit + info ausgeben}
  Show1.Visible := slA.Count >= 1;
  Show2.Visible := slA.Count >= 2;
  Show3.Visible := slA.Count >= 3;
  Show4.Visible := slA.Count >= 4;
  Show5.Visible := slA.Count >= 5;
end;

Ist dir beim Schreiben nicht aufgefallen, daß du da ständig Code kopiert hast?

Delphi-Quellcode:

if Zeilen >= XXX then
//if slA[XXX-1]) <> '' then
 begin
 ShowMessage(Format('Z%d,slA[%d]', [XXX, XXX-1]));
 StringRoh:=slA[XXX-1];
 Zeit:=Copy(StringRoh,1,5);
 Info:=StringRoh;
 if Length(StringRoh) >6 then Delete(Info,1,6);
 (FindComponent('Show'+IntToStr(XXX)) as TLabel).Caption:=Zeit + ' - ' + Info;
 end;

Man glaubt es kaum, sowas kann man entweder in eine eigene Methode auslagern, oder als Schleife zusammenfassen. :roll:

Außerdem verwendest du manchmal "Zeilen" und dann wieder slA.Count, welches doch alles das Selbe ist.
Nja egal, aber wenn schon, dann sollte man sich schon auf Einwas einigen.

Und das

Delphi-Quellcode:

ShowXXX.Visible := slA.Count >= XXX;

lässt sich notfalls auch in eine Schleife legen, oder gleich mit dem

Delphi-Quellcode:

Zeilen >= XXX

abgleichen.
Siehst'e, hier wird auch wieder das Gleiche verwendet, also kann man das sogar an eine Stelle zusammenfassen.

Delphi-Quellcode:

ShowXXX.Visible := Zeilen >= XXX;
if ShowXXX.Visible then
...

Das komplette PrintOutAuto besteht letztendlich nur aus effektiv maximal 15 Codezeilen (eher die Hälfte), aus Welchen jemand fast 150 gemacht hat.
Je mehr Code, um so besser ist er und 5 mal der gleiche Code, das ist dann natürlich 5 Mal so guter Code :angel2:

[Stevie]

Zitat:

Zitat von himitsu (Beitrag 1261988)

Je mehr Code, um so besser ist er und 5 mal der gleiche Code, das ist dann natürlich 5 Mal so guter Code :angel2:

Wenn man nach LoC bezahlt wird schon :stupid:

[himitsu]

Delphi-Quellcode:

if 
Show5
.
Visible 
= 
True 
then
begin
ShowMessage
(
'H'
+
'a'
+
'l'
+
'l'
+
'o'
)
;
end
;

:stupid:


Ach ja, LoC = Lines-of-Code

[Popov]

Geht vermutlich besser und kürzer, aber man sollte nicht übertreiben...

Delphi-Quellcode:

procedure PrintOutAuto2;

  function Toaster(AIndex: Integer): String;
  var
    Zeit: String;
    Info: String;
    StringRoh: String;
  begin
    ShowMessage(Format('Z%d,slA[5d]', [AIndex + 1, AIndex]));
    StringRoh := slA[AIndex];
    Zeit := (Copy(StringRoh, 1, 5));
    Info := StringRoh;
    if Length(StringRoh) > 6 then
      Delete(Info, 1, 6);
    Result := Zeit + ' - ' + Info;
  end;

  procedure Marmelade(ShowX: array of Byte);
  begin
    with Form1 do
    begin
      Show1.Visible := ShowX[0] = 1;
      Show2.Visible := ShowX[1] = 1;
      Show3.Visible := ShowX[2] = 1;
      Show4.Visible := ShowX[3] = 1;
      Show5.Visible := ShowX[4] = 1;
    end;
  end;

var
  Zeit: String;
  Info: String;
  StringRoh: String;
  Zeilen: Integer;
begin
  Zeilen := slA.Count; //Füllen der Zeilen
  ShowMessage(IntTOStr(Zeilen));

  with Form1 do
  begin
    if Zeilen >= 1 then
    //if Length(slA[0]) >= 1 then
      Show1.Caption := Toaster(0)
    else
      Marmelade([0, 0, 0, 0, 0]);

    if Zeilen >= 2 then
    //if Length(slA[1]) >= 1 then
      Show2.Caption := Toaster(1)
    else
      Marmelade([1, 0, 0, 0, 0]);

    if Zeilen >= 3 then
    //if Length(slA[2]) >= 1 then
      Show3.Caption := Toaster(2)
    else
      Marmelade([1, 1, 0, 0, 0]);

    if Zeilen >= 4 then
    //if Length(slA[3]) >= 1 then
      Show4.Caption := Toaster(3)
    else
      Marmelade([1, 1, 1, 0, 0]);

    if Zeilen >= 5 then
    //if Length(slA[4]) >= 1 then
      Show5.Caption := Toaster(4)
    else
      Marmelade([1, 1, 1, 1, 0]);

     {für die ersten 5: zeit + info ausgeben}
    if not Show1.Visible then //Abfangen, falls ShowLabel nicht sichtbar ist
      Show1.Visible := slA.Count >= 1; //Nur aktivieren, wenn wirklich benötigt

    if not Show2.Visible then
      Show2.Visible := slA.Count >= 2;

    if not Show3.Visible then
      Show3.Visible := slA.Count >= 3;

    if not Show4.Visible then
      Show4.Visible := slA.Count >= 4;

    if not Show5.Visible then
      Show5.Visible := slA.Count >= 5;

  end; //with Form1
end;

Natürlich wie immer: ungeprüft

[mkinzler]

Noch mal eine kurze Zusammenfassung:
1.) Mach eine Methode der Form aus der (freien) Prozedur.
2.) Man sollte keine Festen Referenzvariablen referenzieren (Form1).
3.) Vergessse with
4.)

Delphi-Quellcode:

procedure TForm1.Marmelade(ShowX: array of Byte);
var
  i; Integer;
  o: TComponent;
begin
  for i := Low(ShowX) to High(ShowX) do
  begin
    o := FindComponent('Show'+IntToStr(i+1));
    if ShowX[0] = 1 then o.Visible := True else o.Visible := False;
  end;
end;