Code Optimierung gesucht

[Martin W]

Hi,

ich habe folgende Klassen (alles stark vereinfacht und auf das grundlegende Problem reduziert):

Delphi-Quellcode:

type TAuto = class(TComponent)
public
  FNameOfCar: string;
  FColor: integer;
  FNumberOfWheels: integer;
  FLicencePlate: string;
end;

type TAutoHerstellerA = class(TAuto)
public
  FWindowColor: integer;
  FNumberOfDoors: integer;
end;

type TAutoHerstellerB = class(TAuto)
public
  FHupe: boolean;
end;

type TAutoHerstellerC = class(TAuto)
public
  FNiceCar: boolean;
end;

type TAutoHerstellerD = class(TAuto)
public
  FRamboPackage: boolean;
  FAutonomCar: boolean;
end;

Verwaltet werden die Auto-Hersteller A-D in folgenden Klassen:

Delphi-Quellcode:

FAutoHerstellerA: TList<TAutoHerstellerA>;
FAutoHerstellerB: TList<TAutoHerstellerB>;
FAutoHerstellerC: TList<TAutoHerstellerC>;
FAutoHerstellerD: TList<TAutoHerstellerD>;

Nun der interessante Code, ich möchte ein bestimmtes Auto umfärben, jedes Auto existiert nur einmal:

Delphi-Quellcode:

procedure ColorCar(aCar: string, aNewColor: integer);
var
  i: integer;
begin

  for i:=0 to FAutoHerstellerA.Count -1 do
    begin
      if FAutoHerstellerA[i].FNameOfCar = aCar then
        FAutoHerstellerA[i].FColor := aNewColor;
      Break;
    end;

  for i:=0 to FAutoHerstellerB.Count -1 do
    begin
      if FAutoHerstellerB[i].FNameOfCar = aCar then
        FAutoHerstellerB[i].FColor := aNewColor;
      Break;
    end;

  for i:=0 to FAutoHerstellerC.Count -1 do
    begin
      if FAutoHerstellerC[i].FNameOfCar = aCar then
        FAutoHerstellerC[i].FColor := aNewColor;
      Break;
    end;

  for i:=0 to FAutoHerstellerD.Count -1 do
    begin
      if FAutoHerstellerD[i].FNameOfCar = aCar then
        FAutoHerstellerD[i].FColor := aNewColor;
      Break;
    end;

end;

Man sieht schon, je nach Anzahl der Autohersteller habe ich viele Wiederholungen. Gibt es daher die Möglichkeit, die Listen zu einer Art Gruppe zusammenzufassen und die Gruppe zu durchsuchen?

Viele Grüße,
Martin

[mkinzler]

Sind die Klassen wirklich so verschieden, dass man das nicht durch Eigenschaften/Vererbung/Interfaces lösen könnte?

[Martin W]

Die Grundklasse "TAuto" ist überall gleich, der Rest leider nicht. In der Funktion wird nur auf Eigenschaften von TAuto zurückgegriffen. Habe gerade folgende Idee bekommen:

Delphi-Quellcode:

var
  WorkingList: TList<TAuto>;
begin

  WorkingList Create

  WorkingList.Add(alle anderen Listen hinzufügen zur List)

  Kombinierte Liste durchsuchen

  WorkingList wieder freigeben

end;

[Dejan Vu]

Pack doch einfach alle Autos in eine einzige Liste.

[Martin W]

Zitat:

Zitat von Dejan Vu (Beitrag 1285279)

Pack doch einfach alle Autos in eine einzige Liste.

Ja, das hab ich gemacht, wie folgt:

Delphi-Quellcode:

var
  WorkingList: TList<TAuto>;
begin

  WorkingList := WorkingList<TAuto>.Create;

  WorkingList.AddRange(TAutoHerstellerA);
  WorkingList.AddRange(TAutoHerstellerB);
  WorkingList.AddRange(TAutoHerstellerC);
  WorkingList.AddRange(TAutoHerstellerD);

  for i:=0 to WorkingList.Count -1 do
    begin
      if WorkingList[i].FNameOfCar = aCar then
        WorkingList[i].FColor := aNewColor;
      Break;
    end;

  WorkingList.Free;

end;

Nachteil: Die Liste muss vor jeder Nutzung neu erstellt ("kombiniert") werden. Gibt es eine Möglichkeit, die Liste dynamisch zu halten, so dass bei Änderung von TAutoHerstellerA automatisch die WorkingList mit aktualisiert wird? Ok, eine Idee für das wären Properties.

[mkinzler]

Nicht nur zum Suchen, sondern grundsätzlich eine Liste.

[Martin W]

Zitat:

Zitat von mkinzler (Beitrag 1285283)

Nicht nur zum Suchen, sondern grundsätzlich eine Liste.

Die werden ziemlich häufig individuell verarbeitet, nur selten gemeinsam. Daher ist der dadurch entstehende Aufwand höher, als wenn ich sie auseinander lasse.

AddRange klappt im oberen Code nicht, gibt es eine andere Möglichkeit, die Listen zu kombinieren?

[mkinzler]

Durch das Kominieren nur zu diesem Zweck gewinnst Du nichts im Vergleich zur separaten Suche in den getrennten Listen.

[Martin W]

Zitat:

Zitat von mkinzler (Beitrag 1285286)

Durch das Kominieren nur zu diesem Zweck gewinnst Du nichts im Vergleich zur separaten Suche in den getrennten Listen.

Performancetechnisch nichts, aber codemäßig, vor allem wen das nicht 4 Listen sind, sondern 50.

[mkinzler]

Vielleicht ist es möglich statt x Listen einen Array/Directory der Listen zu verwenden, dann kannst Du den Code einfach zusammenfassen.

[Martin W]

Zitat:

Zitat von mkinzler (Beitrag 1285288)

Vielleicht ist es möglich statt x Listen einen Array/Directory der Listen zu verwenden, dann kannst Du den Code einfach zusammenfassen.

Danke für die Idee, werde mir das mal näher anschauen.

[Bjoerk]

Wenn Autohersteller eine so wichtige Eigenschaft ist könnte man überlegen die nach TAuto zu packen? Und ich würde auf jeden Fall auch nur eine Liste nehmen.

[Martin W]

Zitat:

Zitat von Bjoerk (Beitrag 1285290)

Wenn Autohersteller eine so wichtige Eigenschaft ist könnte man überlegen die nach TAuto zu packen? Und ich würde auf jeden Fall auch nur eine Liste nehmen.

Ja, ich bin gerade am prüfen, wären fast 25000 Zeilen Code die ich prüfen & nachziehen müsste.

[mkinzler]

Würde ich auch. Ihm scheinen aber die getrennten Listen wichtig zu sein.

[Martin W]

Zitat:

Zitat von mkinzler (Beitrag 1285292)

Würde ich auch. Ihm scheinen aber die getrennten Listen wichtig zu sein.

Ja, darauf wird es wohl hinauslaufen! Danke für eure Antworten.

[Martin W]

OK, angenommen ich habe eine gesamte Liste

Delphi-Quellcode:

AutoListe: TList<TAuto>

.

Gibt es eine effizente Möglichkeit, zum Beispiel alle Items anzeigen zu lassen, die vom Typ TAutoHerstellerA (

Delphi-Quellcode:

AutoListe[n] is TAutoHerstellerA

) sind?

[glotzer]

An sich spricht doch auch nix dagegen dass ein Auto in mehreren Listen gleichzeitig ist oder? Du kannst ja deine einzelnen Listen beibehalten und eben zusätzlich noch eine große gemeinsame Liste führen.

[Martin W]

Zitat:

Zitat von glotzer (Beitrag 1285295)

An sich spricht doch auch nix dagegen dass ein Auto in mehreren Listen gleichzeitig ist oder? Du kannst ja deine einzelnen Listen beibehalten und eben zusätzlich noch eine große gemeinsame Liste führen.

Ja, hab es jetzt mit Properties gelöst. Beim Zugriff auf die Variable wird die Meta-Liste jeweils neu zusammmengebaut.

[Bjoerk]

Hast du die Idee von Markus mit dem Array probiert?

Delphi-Quellcode:

TAutoHersteller = (ahVW, ahMBW, ahBenz)

TAutoList = class(TObjectList)

TAutos = array[TAutoHersteller] of TAutoList;

for AutoHersteller := Low(TAutoHersteller) to High(TAutoHersteller) do
 for I := 0 to FAutos[AutoHersteller].Count - 1 do
   if FAutos[AutoHersteller].Items[I].NameOfCar = aCar then

[Dejan Vu]

Also ich würde wirklich einen einzigen Container für alle Autos verwenden. Falls nur über Autos eines bestimmten Typen iteriert werden soll, dann spendiert man dem Container eine Methode/Funktion, mit der man durch alle Autos eines bestimmten Typen iterieren kann.

Delphi-Quellcode:

Type
  myCarContainer : TCarContainer;

Begin
  myCarContainer := TCarContainer.Create;
  myCarContainer.Add(THerstellerA.Create);
  ...
  ...

Var
  myIterator : TCarIterator<THerstellerA>;
  allIterator : TCArIterator<TAuto>;
begin
  myIterator := myCarContainer.GetCars<THerstellerA>;
  while myIterator.Next do
    DoSomethingWith (myIterator.Current);
  ...
  allIterator := myCarContainer.GetCars<TAuto>;
  while allIterator.Next do
    myIterator.Current.Color := NeueFarbe;
  ...

Man könnte sich vom Container auch gleich die ganze Liste geben lassen, aber dann hätte man die Instanzen redundant gespeichert, was unnötig ist.

Natürlich soll man den bestehenden Code nicht umschreiben, es kann also wirklich sinnvoll sein, die Listen aus dem Container zu extrahieren, um die betreffenden Codestellen nicht zu sehr umschreiben zu müssen. Allerdings glaube ich, das man hier mit Refaktoring sehr viel vereinfachen kann, denn irgendwie scheinen die ganzen Listen doch eh nur immer wieder durchlaufen zu werden. Und das geht auch sehr elegant mit einer ForEach-Methode:

Delphi-Quellcode:

...
myCarContainer.ForEach<THerstellerA>(ChangeColor);
...

Function ChangeColor (item : THerstellerA) : Boolean;
Begin
  item.Color := ColorToChange;
  Result := True; // False würde den 'ForEach' Operator abbrechen
End;

Das kann man noch beliebig weiterspinnen. Wichtig ist hier, das man zuerst die Funktionalität definiert, d.h. das Interface und das dann implementiert. Und das Interface richtet sich eben u.a. daran, möglichst wenig am Code kaputt zu optimieren.

[alda]

Unabhängig von den Listen solltest Du auf jeden Fall auch deine Klasen-Struktur überdenken - aber ich vermute, dass Dein Code schon Älter ist und eine Anpassung sehr aufwendig ist. Hier ein paar Ideen:

• auf keinen Fall mehrere Listen - damit ist jede verwendete Codestelle anzupassen, sofern ein neuer Hersteller hinzukommt. (da schließ ich mich dem Rest an)
• Warum von TComponent ableiten? Egal für welchen aktuellen Zweck erachte ich das als nicht sinnvoll.
• Ableitungen pro Autohersteller halte ich im allgemeinen auch nicht für sinnvoll
• die Ableitungen pro Hersteller scheinen aufgrund der als bool (vereinfachten) Eigenschaften vorgenommen worden zu sein, sollten wiederum aber auch eigene Klassen sein (ich nenne Sie mal TAusstattung), da die Vererbung die Flexibilität hier stark eingrenzt.
• Einsatz von Interfaces für bestmögliche Entkopplung und Testbarkeit

[Dejan Vu]

Zitat:

Zitat von alda (Beitrag 1285306)

...

• Ableitungen pro Autohersteller halte ich im allgemeinen auch nicht für sinnvoll

...

Sind sie aber. Alles andere (Enum/ Eigenschaft 'Fahrzeugtyp') bedeutet auch: Verletzung des OCP, d.h. die 'Auto-Klasse' müsste jedes mal angefasst werden, wenn eine neue Type mit spezifischen Eigenschaften hinzukommt. Hast Du dagegen pro Hersteller eine eigene Klasse, bleiben alle existierenden Klassen beim Hinzufügen einer neuen Klasse unangetastet, was den Code wesentlich robuster, d.h. stabiler macht.

Nur wenn es je Hersteller garantiert keine individuellen Eigenschaften gäbe (was hier nicht er Fall ist), muss man keine einzelne Klassen bilden.

EDIT: Habe deinen Post gerade noch einmal gelesen: Du meinst, ein TAuto, aber je Hersteller unterschiedliche Ausstattungen? Das wäre eine Möglichkeit, wobei diese Architektur impliziert, das man den Hersteller wechseln kann. Weiterhin müsste man die Eigenschaft 'Ausstattung' jedes Mal casten, um auf spezifische Eigenschaften eines Herstellers zu gelangen.
Ferner müsste die Zuordnung 'Fahrzeugtyp => Ausstattungsklasse' in einer Factory erfolgen, die der Konstruktor der Klasse TAuto aufruft, damit OCP nicht verletzt wird:

Delphi-Quellcode:

Type
  TAuto = Class
    FHersteller : THersteller;
    FAusstattung : TAusstattung;
  public
    constructor Create (aHersteller : THersteller);
    Property Ausstattung : TAusstattung read FAusstattung;
  end;

...
constructor TAuto.Create(aHersteller : THersteller);
begin
  FAusstattung := TAusstattungFactory.Create(aHersteller);
end;

[alda]

Zitat:

Zitat von Dejan Vu (Beitrag 1285308)

Zitat:

Zitat von alda (Beitrag 1285306)

...

• Ableitungen pro Autohersteller halte ich im allgemeinen auch nicht für sinnvoll

...

Sind sie aber. Alles andere (Enum/ Eigenschaft 'Fahrzeugtyp') bedeutet auch: Verletzung des OCP, d.h. die 'Auto-Klasse' müsste jedes mal angefasst werden, wenn eine neue Type mit spezifischen Eigenschaften hinzukommt. Hast Du dagegen pro Hersteller eine eigene Klasse, bleiben alle existierenden Klassen beim Hinzufügen einer neuen Klasse unangetastet, was den Code wesentlich robuster, d.h. stabiler macht.

Nur wenn es je Hersteller garantiert keine individuellen Eigenschaften gäbe (was hier nicht er Fall ist), muss man keine einzelne Klassen bilden.
[/DELPHI]

Da stimme ich Dir zu, allerdings sehe ich den Hersteller eher als Baustein, weniger als Herzstueck der Auto-Klasse an. Die Frage wäre welche Informationen des Herstellers benötigt werden und was damit im weiteren Verlauf ermittelt werden soll.

Zitat:

Zitat von Dejan Vu (Beitrag 1285308)

EDIT: Habe deinen Post gerade noch einmal gelesen: Du meinst, ein TAuto, aber je Hersteller unterschiedliche Ausstattungen? Das wäre eine Möglichkeit, wobei diese Architektur impliziert, das man den Hersteller wechseln kann. Weiterhin müsste man die Eigenschaft 'Ausstattung' jedes Mal casten, um auf spezifische Eigenschaften eines Herstellers zu gelangen.
Ferner müsste die Zuordnung 'Fahrzeugtyp => Ausstattungsklasse' in einer Factory erfolgen, die der Konstruktor der Klasse TAuto aufruft, damit OCP nicht verletzt wird:

Ja, oder so ähnlich. Die Frage ist, ob er in seinem Programm die Ausstattungen je Auto konfigurieren kann und welche Rolle der Hersteller spielt. Letztendlich gibt es ja auch oft den Fall, dass Hersteller A ein Auto (teilweise) anfertigt und dann Hersteller B dieses fertigstellt (z.B. individuelle Innen-Austattung) und sein Logo draufsetzt (z.B. Mercedes CITAN).
Ich sehe daher Auto, Hersteller und Ausstattung relativ abstrakt und flexibel - es muss m.E. lediglich an einer Stelle entschieden werden, wer mit wem verheiratet werden darf (z.B.: Welche Ausstattungen können für ein Auto X von Hersteller Y verbaut werden). Das könnte dann z.B. im THersteller untergebracht werden (quasi ein Ausstattungsprofil) - die komplexere Logik, welche Ausstattung eine andere aussticht oder ersetzt, sollte dann wiederum ausgelagert werden.

Aber wie immer finde ich es im allgemeinen schwierig konkrete Vorschläge zu geben, wenn wir den genauen Anwendungsfall nicht kennen. Des Weiteren glaube ich, dass es unmöglich ist den Code aufzuräumen, wenn alleine die Anpassung der Listenverarbeitungen so lange dauern sollte wie der TE vermutet hat.

Und was genau meinst Du mit "... müsste man die Eigenschaft 'Ausstattung' jedes Mal casten ...", stehe grad auf dem Schlauch?

[Dejan Vu]

Zitat:

Zitat von alda (Beitrag 1285321)

Und was genau meinst Du mit "... müsste man die Eigenschaft 'Ausstattung' jedes Mal casten ...", stehe grad auf dem Schlauch?

z.B.:

Delphi-Quellcode:

Var
  car : TCar;
...
car := myCarContainer.GetCars(herstellerMercedes).First; 

if (car.Ausstattung as TMercedesausstattung).Sitzheizung.Status=AufStufe2 then
  CheckTemperature(my.Back.Lower);

...

habe ich konkrete Autoklassen, liefert 'GetCars' direkt den konkreten Typen, d.h. ich muss nirgens casten.

Delphi-Quellcode:

Var
  mercedes : TMercedes;

...
mercedes := myCarContainer.GetCars<TMercedes>.First;

if mercedes.Ausstattung.Sitzheizung.Status=AufStufe2 then
  CheckTemperature(my.Back.Lower);

Hier sind alle Typen konkret.

Grundsätzlich bevorzuge ich die 2.Möglichkeit (ohne Typecasting auskommen), wobei das nicht notwendigerweise 'pro Autoklasse' bedeutet, aber hier zufälligerweise so ist.

[Sir Rufo]

Ich frage mich inwieweit das Beispiel mit der realen Anforderung übereinstimmt oder ob hier nur einfach ein unglückliches Beispiel gewählt wurde und es somit zu falschen Annahmen und Schlußfolgerungen kommt.

[alda]

Ach so meinst Du das, ja da stimme ich Dir auch zu. Ich wollte auch nur einen Denkansporn liefern, über konkrete Implementierungen (Dein Beispiel wäre mir je nach Anwendungsfall auch schon wieder zu starr) kann dann disktutiert werden, wenn wir wissen was der TE alles mit dem Auto und vorallem den Ausstattungen machen können muss - oder hab ich das irgendwo überlesen?

Aber wie bereits erwähnt vermute ich, dass Änderungen eher schwierig sind für den TE - es handelt sich ja bestimmt auch um ein Programm das bereits produktiv im Einsatz ist und nicht mal eben umgestaltet werden kann?

Edit:

Zitat:

Zitat von Sir Rufo (Beitrag 1285326)

Ich frage mich inwieweit das Beispiel mit der realen Anforderung übereinstimmt oder ob hier nur einfach ein unglückliches Beispiel gewählt wurde und es somit zu falschen Annahmen und Schlußfolgerungen kommt.

Ja, genau das ist die Frage :>

[Martin W]

Danke euch sehr, vor allem das mit dem letzten Beitrag schaue ich mir näher an!

Viele Grüße!