Problem bei TStringList

[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.

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 ShowXXX.Visible := slA.Count >= XXX; lässt sich notfalls auch in eine Schleife legen, oder gleich mit dem 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

[Stevie]

Zitat von himitsu:

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

Wenn man nach LoC bezahlt wird schon

[himitsu]

Delphi-Quellcode:

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

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;

[Popov]

Zitat von mkinzler:

3.) Vergessse with

In dem Fall sehe ich nichts verwerfliches dran, aber jeder nach seinem Geschmack.

Zitat:

4.)

Delphi-Quellcode:

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

[mkinzler]

In diesem Fall nicht, aber es ist unnötig, wenn er eine Methode daraus macht.