Klassendesign. Frage zu Property Settern was sie dürfen und was nicht

[Bünni]

Mir stellt sich gerade eine Frage was mein Klassendesign und das meiner Property Setter angeht.
Ich versorge mein VirtualStringTree so wie das sein soll mit einer ObjectList und die hat wieder Klasseninstanzen intern gespeichert.

Meine Klasse hat ein Property namens IconIndex. Bis vor etwa 2 Stunden habe ich das IconIndex in etwa so gesetzt

Delphi-Quellcode:

procedure TKSTDataFunctions.SetIcon(const DataIndex, IconIndex: Integer);
begin
 TKSTData.KSTData[DataIndex].IconIndex := IconIndex;
end;

Natürlich mit Überprüfungen, ob DataIndex existiert usw.

Jeder KSTData-Instanz, also eigentlich der VST Node im Endeffekt, konnte einen von mehreren Status haben. Das Icon signalisiert den Status.
Meine alte Methode hat funktioniert.

Ich habe eben aber ein weiteres Property FStatus in KSTData eingebaut. Der Typ ist ein Enum

Delphi-Quellcode:

TStatus = (tsCalculating, tsError);

Setze ich jetzt mein Property, setze ich im Setter des Properties gleichzeitig den IconIndex

Delphi-Quellcode:

IconIndex := Ord(TStatus(Ord(Value)));

Im Setter von IconIndex invalidiere ich das VST

Delphi-Quellcode:

TKSTForm.VST.Invalidate;

Ich brauche also nur noch den Status (Enum) zu setzen, und der Rest passiert quasi automatisch in den Settern statt umständlich vorher mit manuellem Setzen des IconIndex.
Das Setzen des Enums hat meiner Meinung nach den Vorteil, dass ich später prüfen kann, welchen Status ein Node und dessen Daten haben. Das über einen Integer-Index abzufragen stelle ich mir schlecht vor.


Meine Frage
Darf man das? Oder gilt das als unsauberer Code, in den Settern von Properties andere Properties zu setzen?
Wenn ich beispielsweise im Setter von Status, also SetStatus, je nachdem welcher Status gesetzt werden soll, in einem Case-Block den IconIndex (ohne F, sonst gehts nicht in den Setter von IconIndex) setze - ist sowas in Ordnung?

Ich würde meine Änderung schon gerne behalten, da ich so mit nur einer Zeile Code gleich mehrere Änderungen vollziehe und keine weiteren Units mehr brauche, die mir den IconIndex von Außen setzen.
So arbeite ich objekt-orientiert, direkt am Objekt, und ändere seine Daten.

Wenn ich nicht das Objekt direkt zur Verfügung habe, dafür aber einen Index oder einen Namen kann ich das hier aufrufen

Delphi-Quellcode:

procedure TKSTDataFunctions.SetKSTState(const DataIndex: Integer; Status: TStatus);
begin
 if (DataIndex> -1) and (DataIndex < TKSTData.KSTData.Count) then
  TKSTData.KSTData[DataIndex].KSTStatus := Status;
end;

procedure TKSTDataFunctions.SetKSTState(const KSTDataName: string; Status: TStatus);
begin
 SetKSTState(TKSTDataFunctions.FindIDByName(KSTDataName), Status);
end;

Oft werde ich die beiden Helferfunktionen da oben nicht brauchen, da ich immer das Objekt habe. Aber besser sie liegen ungenutzt rum und wenn ich sie irgendwann brauche, sind sie da.

[stahli]

Also soweit ich das verstehe...

Im Grundsatz ist es unproblematisch, in einem Setter auch andere Eigenschaften zu definieren. Dafür sind die Setter ja genau da.

Ob Du eine Eigenschaft von der Liste aus mit einem Index setzt oder direkt am Datenobjekt bleibt sich auch gleich. Die Liste würde ja auch nur das Datenobjekt ermitteln und diesem den Änderungsauftrag übergeben.
Es ist nur eine Frage der Businesslogik, von wo aus die Änderung veranlasst wird.

Was mir etwas unklar erscheint ist die mögliche Redundanz von State und Icon in ein er Klasse. Bezeichnen die beiden nicht exakt das Gleiche?


Also grundsätzlich sehe ich bei Deinem Ansatz kein Problem, aber ich weiß nicht, ob ich alles wirklich richtig verstanden habe.

[Bünni]

State enthält einem Enum. Icon enthält den eigentlichen Integer Index für die ImageList den ich einmalig im Setter aus dem übergebenem Enum ermittle.

Ich habe diese beiden Properties der Einfachheit halber gewählt.
Da wo ich den IconIndex brauche (lesend), kann ich direkt auch auf IconIndex zugreifen und erhalte den Integer Wert zurück.
Wo ich nur den Status prüfen muss (lesend), kann ich State auslesen und erhalte ein Enum zum Vergleich zurück.

Delphi-Quellcode:

TStatus = (tsCalculating, tsError

Wenn ich State auf tsError setze, dann bekommt IconIndex im SetKSTState-Setter den Wert Ord(Value);
Das hat wie oben schon erwähnt den Vorteil, dass ich direkt an das IconIndex als Integer komme und nicht erst umrechnen müsste, welches Element es in TStatus ist.

Die Redundanz habe ich also entweder im Klassenobjekt einmalig, oder mehrfach dort, wo ich sonst das IconIndex brauche und erst vom State umrechnen müsste.
IconIndex wird von Außen nur gelesen, nicht gesetzt.


Kurz zusammengefasst könnte man meine Frage auch so schreiben:
"darf ein Property 1 in seinem Setter ein anderes Property 2 ändern?"

Die frage hast du mir beantwortet. Damit ist für mich jetzt klar, dass das kein Problem ist.

[Aviator]

Also ich sehe das so, dass du den IconIndex gar nicht setzen brauchst. Es gibt auch Readonly Properties die nur einen Getter haben. Dann würde es reichen, einfach im Getter der IconIndex Property anhand des Status den richtigen Index zurückzugeben. Vorteil hier ist, dass du im SetStatus nur die Variable für den eigentlichen Status ändern musst, die Property sich nur um sich selbst kümmern muss (keine größeren Abhängigkeiten schaffen) und nicht noch durch irgendeine Setter Logik plötzlich eine andere Property geändert wird. Und du hast eine Variable weniger im Objekt.

Um das aber ganz genau zu machen, dürfte dein Objekt gar keine IconIndex Property besitzen. Das Objekt geht es nämlich nichts an, wie der Status später angezeigt wird. Ob das ein TEdit ist in dem ein anderer Text erscheint oder ein Image das die Farbe verändert spielt dann keine Rolle mehr. Alle orientieren sich nur noch am Status Flag des Objekts. Und gerade beim VirtualTreeView ist das ja relativ einfach. Einfach InvalidateNode() aufrufen und im OnGetImageIndex() bzw. On[Before|After]CellPaint() Event anhand des Status den entsprechenden Index der aktuellen ImageList ermitteln und zeichnen lassen.

Probleme würdest du schon bekommen, wenn du deine Klasse an zwei verschiedenen Stellen im Programm verwenden wolltest die zwei unterschiedliche ImageLists benutzen. Dann wäre nämlich deine komplette Speicherung des ImageIndex im Objekt nutzlos.

[Bünni]

Zitat:

lso ich sehe das so, dass du den IconIndex gar nicht setzen brauchst. Es gibt auch Readonly Properties die nur einen Getter haben. Dann würde es reichen, einfach im Getter der IconIndex Property anhand des Status den richtigen Index zurückzugeben.

Werde ich mir ansehen.

Zitat:

Um das aber ganz genau zu machen, dürfte dein Objekt gar keine IconIndex Property besitzen.

Zitat:

Und gerade beim VirtualTreeView ist das ja relativ einfach. Einfach InvalidateNode() aufrufen und im OnGetImageIndex() bzw. On[Before|After]CellPaint() Event anhand des Status den entsprechenden Index der aktuellen ImageList ermitteln und zeichnen lassen.

Wenn ich hier anhand des Status den Index ermitteln möchte, muss ich aber statt KSTData.IconIndex > ord(KSTData.Status) verwenden. Ist das so richtig?

[Aviator]

Naja wenn du dir sicher bist, dass in deiner ImageIndex die Icons immer an den Positionen 0..n liegen kannst du das so machen. Das bedeutet im Umkehrschluss aber auch, dass du bei einem neuen Status immer etwaige andere Icons verschieben und dann auch im Code die Änderungen nachziehen musst. Es sei denn, die ImageList wird nur für diese Status Images benutzt. Aber auch das halte ich für eine etwas unsaubere Implementierung.

Lagere die Ermittlung des ImageIndex lieber in eine eigene Funktion aus. Dann hast du das zentral gespeichert und bist total flexibel. Zudem verkürzt sich dein OnGetImageIndex() auf eine einzelne Zeile zur Ermittlung des ImageIndex (abgesehen vom Abrufen des NodeData Objekts). Zudem erhälst du in dem Beispiel auch direkt eine Exception statt eines falschen ImageIndex wenn du einen neuen Status einführst und vergisst, diesen in der GetImageIndexFromStatus() Methode nachzuziehen.

Delphi-Quellcode:

function GetImageIndexFromStatus(const AStatus: TStatus);
begin
  Result := -1;
  
  case AStatus of
    tsCalculating: Result := 0;
    tsError: Result := 1;
    tsSomeNewStatus: Result := 37;
    else
      raise ENotImplemented.CreateFmt('StatusCode %d not implemented.', [Ord(AStatus)])
  end;
end;

[Bünni]

Zitat:

Naja wenn du dir sicher bist, dass in deiner ImageIndex die Icons immer an den Positionen 0..n liegen kannst du das so machen. Das bedeutet im Umkehrschluss aber auch, dass du bei einem neuen Status immer etwaige andere Icons verschieben und dann auch im Code die Änderungen nachziehen musst. Es sei denn, die ImageList wird nur für diese Status Images benutzt. Aber auch das halte ich für eine etwas unsaubere Implementierung.

Deswegen habe ich den IconIndex im Setter oben ja auf Ord(Value [State]) gesetzt. Somit war er immer gleich. Das Problem mit sich verschiebenen Indexes hatte ich nie.

Zitat:

Zudem verkürzt sich dein OnGetImageIndex()

Ich male meine Icons selber. OnGetImageIndex benutze ich nicht.

Helferfunktionen werde ich gleich einbauen so wie du vorgeschlagen hast. Das klingt gut und so werde ich auch ein paar Properties los.


@Aviator
deine Änderungen sind eingebaut. Alle Abhängigkeiten sind verschwunden, Helferfunktionen eingebaut und IconIndex ist auch weg.
Änderungen am Klassenobjekt
- 3 Feldvariablen weniger
- 6 Properties weniger
- 36 Zeilen Code ohne Leerzeilen weniger
- 3 Helferfunktionen sind dazugekommen die all die vorherige Arbeit in jeweils einer Zeile erledigen.

Die Unit mit meinem Klassenobjekt hat zwar noch immer 953 Zeilen mit Leerzeilen und ein paar Kommentaren, aber da steckt auch viel drin. Zusammen 92 Properties.
Keine Redundanzen, keine doppelten Properties. Alles wichtige Dinge für den Programmablauf.