Wie man gegen diese „vollständig öffentliche“ Denkweise des Entwurfs von Geschäftsobjektklassen argumentiert

9

Wir führen viele Unit-Tests und Refactoring-Vorgänge für unsere Geschäftsobjekte durch, und ich habe anscheinend ganz andere Meinungen zum Klassendesign als andere Kollegen.

Eine Beispielklasse, von der ich kein Fan bin:

public class Foo
{
    private string field1;
    private string field2;
    private string field3;
    private string field4;
    private string field5;

    public Foo() { }

    public Foo(string in1, string in2)
    {
        field1 = in1;
        field2 = in2;
    }

    public Foo(string in1, string in2, string in3, string in4)
    {
        field1 = in1;
        field2 = in2;
        field3 = in3;
    }

    public Prop1
    { get { return field1; } }
    { set { field1 = value; } }

    public Prop2
    { get { return field2; } }
    { set { field2 = value; } }

    public Prop3
    { get { return field3; } }
    { set { field3 = value; } }

    public Prop4
    { get { return field4; } }
    { set { field4 = value; } }

    public Prop5
    { get { return field5; } }
    { set { field5 = value; } }

}

In der "echten" Klasse sind sie nicht alle Zeichenfolgen, aber in einigen Fällen haben wir 30 Hintergrundfelder für vollständig öffentliche Eigenschaften.

Ich hasse diese Klasse und ich weiß nicht, ob ich nur wählerisch bin. Ein paar bemerkenswerte Dinge:

  • Private Sicherungsfelder ohne Logik in den Eigenschaften scheinen unnötig und blähen die Klasse auf
  • Mehrere Konstruktoren (etwas in Ordnung), aber gekoppelt mit
  • Alle Immobilien haben einen öffentlichen Setter, ich bin kein Fan.
  • Möglicherweise wird aufgrund des leeren Konstruktors keinen Eigenschaften ein Wert zugewiesen. Wenn ein Aufrufer dies nicht weiß, kann dies möglicherweise zu unerwünschten und schwer zu testenden Verhaltensweisen führen.
  • Es sind zu viele Eigenschaften! (im Fall 30)

Ich finde es viel schwieriger , als Implementierer wirklich zu wissen, in welchem ​​Zustand sich das Objekt Foozu einem bestimmten Zeitpunkt befindet. Das Argument lautete: "Wir haben möglicherweise nicht die erforderlichen Informationen, um sie Prop5zum Zeitpunkt der Objektkonstruktion festzulegen. Ok, ich denke, ich kann das verstehen, aber wenn dies der Fall ist, machen Sie nur Prop5Setter öffentlich, nicht immer bis zu 30 Eigenschaften in einer Klasse."

Bin ich nur wählerisch und / oder verrückt danach, eine Klasse zu wollen, die "einfach zu bedienen" ist, anstatt "einfach zu schreiben (alles öffentlich)"? Klassen wie die oben genannten schreien mir zu, ich weiß nicht, wie das verwendet werden soll, also werde ich einfach alles für alle Fälle öffentlich machen .

Wenn ich nicht besonders wählerisch bin, was sind gute Argumente, um diese Art des Denkens zu bekämpfen? Ich bin nicht sehr gut darin, Argumente zu artikulieren, da ich sehr frustriert bin, wenn ich versuche, meinen Standpunkt zu vermitteln (natürlich nicht absichtlich).

Kritner
quelle
3
Ein Objekt sollte immer in einem gültigen Zustand sein. Das heißt, Sie sollten keine teilweise instanziierten Objekte benötigen . Einige Informationen sind nicht Teil des Kernzustands eines Objekts, während andere Informationen vorhanden sind (z. B. muss ein Tisch Beine haben, aber das Tuch ist optional). Optionale Informationen können nach der Instanziierung bereitgestellt werden. Kerninformationen müssen bei der Instanziierung bereitgestellt werden. Wenn einige Informationen von zentraler Bedeutung sind, aber bei der Instanziierung nicht bekannt sind, muss die Klasse umgestaltet werden. Es ist schwierig, mehr zu sagen, ohne den Kontext der Klasse zu kennen.
Marvin
1
Wenn ich sage "Wir haben möglicherweise nicht die erforderlichen Informationen, um Prop5 zum Zeitpunkt der Objektkonstruktion festzulegen", frage ich: "Sagen Sie, dass es einige Zeiten geben wird, in denen Sie diese Informationen zum Zeitpunkt der Konstruktion haben, und andere Zeiten, in denen Sie dies nicht tun." Haben Sie diese Informationen? " Ich frage mich, ob es tatsächlich zwei verschiedene Dinge gibt, die diese eine Klasse darzustellen versucht, oder ob diese Klasse in kleinere Klassen unterteilt werden sollte. Aber wenn es "nur für den Fall" gemacht wird, ist das auch schlecht, IMO; das sagt mir, dass es kein klares Design dafür gibt, wie Objekte erstellt / gefüllt werden.
Dr. Wily's Apprentice
3
Gibt es einen Grund, warum Sie für die privaten Sicherungsfelder ohne Logik in den Eigenschaften keine automatischen Eigenschaften verwenden ?
Carson63000
2
@Kritner Der springende Punkt bei Auto-Eigenschaften ist, dass Sie, wenn Sie in Zukunft tatsächliche Logik benötigen, diese nahtlos anstelle von Auto hinzufügen können getoder set:-)
Carson63000

Antworten:

10

Vollständig öffentliche Klassen haben eine Rechtfertigung für bestimmte Situationen sowie für die anderen extremen Klassen mit nur einer öffentlichen Methode (und wahrscheinlich vielen privaten Methoden). Und Klassen mit einigen öffentlichen, einigen privaten Methoden.

Es hängt alles von der Art der Abstraktion ab, die Sie mit ihnen modellieren werden, welche Ebenen Sie in Ihrem System haben, welchen Grad an Kapselung Sie in den verschiedenen Ebenen benötigen und (natürlich) welche Denkschule der Autor der Klasse hat von. Sie finden alle diese Typen im SOLID-Code.

Es gibt ganze Bücher darüber, wann welche Art von Design bevorzugt werden soll, daher werde ich hier keine Regeln dafür auflisten, der Platz in diesem Abschnitt würde nicht ausreichen. Wenn Sie jedoch ein Beispiel aus der Praxis für eine Abstraktion haben, die Sie gerne mit einer Klasse modellieren, wird Ihnen die Community hier sicherlich gerne dabei helfen, das Design zu verbessern.

Um Ihre anderen Punkte anzusprechen:

  • "Private Backing-Felder ohne Logik in den Eigenschaften": Ja, Sie haben Recht, für triviale Getter und Setter ist dies nur unnötiges "Rauschen". Um diese Art von "Aufblähen" zu vermeiden, verfügt C # über eine Verknüpfungssyntax für Methoden zum Abrufen / Festlegen von Eigenschaften:

Also statt

   private string field1;
   public string Prop1
   { get { return field1; } }
   { set { field1 = value; } }

schreiben

   public string Prop1 { get;set;}

oder

   public string Prop1 { get;private set;}
  • "Mehrere Konstruktoren": Das ist an sich kein Problem. Es tritt ein Problem auf, wenn dort unnötige Codeduplizierungen vorhanden sind, wie in Ihrem Beispiel gezeigt, oder wenn die aufrufende Hierarchie verschlungen ist. Dies kann leicht gelöst werden, indem gemeinsame Teile in eine separate Funktion umgestaltet werden und die Konstruktorkette unidirektional organisiert wird

  • "Möglicherweise wird aufgrund des leeren Konstruktors keinen Eigenschaften ein Wert zugewiesen": In C # hat jeder Datentyp einen klar definierten Standardwert. Wenn Eigenschaften in einem Konstruktor nicht explizit initialisiert werden, wird ihnen dieser Standardwert zugewiesen. Wenn dies absichtlich verwendet wird , ist es vollkommen in Ordnung - ein leerer Konstruktor könnte also in Ordnung sein, wenn der Autor weiß, was er tut.

  • "Es sind zu viele Immobilien! (Im Fall 30)": Ja, wenn Sie die Freiheit haben, eine solche Klasse auf der grünen Wiese zu entwerfen, sind 30 zu viele, stimme ich zu. Allerdings hat nicht jeder von uns diesen Luxus (haben Sie nicht im Kommentar unten geschrieben, dass es sich um ein Legacy-System handelt?). Manchmal müssen Sie Datensätze aus einer vorhandenen Datenbank oder Datei oder Daten aus einer Drittanbieter-API Ihrem System zuordnen. In diesen Fällen könnten 30 Attribute etwas sein, mit dem man leben muss.

Doc Brown
quelle
Danke, ja, ich weiß, dass POCO / POJOs ihren Platz haben, und mein Beispiel ist ziemlich abstrakt. Ich glaube jedoch nicht, dass ich genauer werden könnte, ohne auf einen Roman einzugehen.
Kritner
@Kritner: siehe meine Bearbeitung
Doc Brown
Ja, normalerweise gehe ich beim Erstellen einer brandneuen Klasse den Weg des automatischen Eigentums. Die privaten Hintergrundfelder "nur weil" zu haben, ist (meiner Meinung nach) nicht erforderlich und verursacht sogar Probleme. Wenn Sie ~ 30 Eigenschaften für eine Klasse und mehr als 100 Klassen haben, ist es nicht weit hergeholt zu sagen, dass es einige Eigenschafteneinstellungen geben oder vom falschen Hintergrundfeld kommen wird ... Ich habe dies bereits mehrmals in unserem Refactoring festgestellt :)
Kritner
danke, der Standardwert für string nullist, nicht wahr? Wenn also eine der Eigenschaften, die tatsächlich in a verwendet wird .ToUpper(), aber niemals ein Wert dafür festgelegt wird, eine Laufzeitausnahme auslöst. Dies ist ein weiteres gutes Beispiel dafür, warum die erforderlichen Daten für die Klasse sollten haben bei der Objektkonstruktion festgelegt werden. Überlassen Sie es nicht nur dem Benutzer. DANKE
Kritner
Neben zu vielen nicht festgelegten Eigenschaften besteht das Hauptproblem bei dieser Klasse darin, dass sie über eine Nulllogik (ZRP) verfügt und der Archetyp für ein anämisches Modell ist. Ohne die Notwendigkeit, dies in Worten auszudrücken, wie z. B. ein DTO, ist es ein schlechtes Design.
user949300
2
  1. Wenn Sie sagen, dass "Private Backing-Felder ohne Logik in den Eigenschaften unnötig erscheinen und die Klasse aufblähen", haben Sie bereits ein anständiges Argument.

  2. Es scheint nicht, dass hier mehrere Konstruktoren das Problem sind.

  3. Wenn Sie alle Eigenschaften als öffentlich haben möchten ... Wenn Sie jemals den Zugriff auf all diese Eigenschaften zwischen Threads synchronisieren wollten, würde es Ihnen schwer fallen, Synchronisationscode überall dort zu schreiben, wo sie sich befinden benutzt. Wenn jedoch alle Eigenschaften von Gettern / Setzern eingeschlossen wären, könnten Sie die Synchronisation problemlos in die Klasse einbauen.

  4. Ich denke, wenn Sie "schwer zu testendes Verhalten" sagen, spricht das Argument für sich. Ihr Test muss möglicherweise überprüfen, ob er nicht null oder ähnliches ist (jeder einzelne Ort, an dem die Eigenschaft verwendet wird). Ich weiß es nicht wirklich, weil ich nicht weiß, wie Ihr Test / Ihre Anwendung aussieht.

  5. Sie haben viel zu viele Eigenschaften, könnten Sie versuchen, die Vererbung zu verwenden (wenn die Eigenschaften ähnlich genug sind) und dann eine Liste / ein Array mit Generika haben? Dann könnten Sie Getter / Setter schreiben, um auf die Informationen zuzugreifen und die Interaktion mit allen Eigenschaften zu vereinfachen.

Schnüffeln
quelle
1
Danke - Nr. 1 und Nr. 4 Ich fühle mich definitiv am wohlsten, wenn ich meine Argumente vorbringe. Ich bin mir nicht ganz sicher, was du mit # 3 meinst. # 5 Die meisten Eigenschaften in den Klassen bilden den Primärschlüssel in der Datenbank. Wir verwenden wettbewerbsfähige Schlüssel, manchmal bis zu 8 Spalten - aber das ist ein anderes Problem. Ich habe darüber nachgedacht, die Teile, aus denen der Schlüssel besteht, in eine eigene Klasse / Struktur zu integrieren, wodurch viele Eigenschaften (zumindest in dieser Klasse) eliminiert würden - aber dies ist eine Legacy-Anwendung mit Tausenden von Codezeilen mit mehreren Anrufer. Ich denke, ich muss viel darüber nachdenken :)
Kritner
Eh. Wenn die Klasse unveränderlich wäre, gäbe es keinen Grund, Synchronisationscode in die Klasse zu schreiben.
RubberDuck
@ RubberDuck Ist diese Klasse unveränderlich? Was meinen Sie?
Snoop
3
Es ist definitiv nicht unveränderlich @StevieV. Jede Klasse mit Eigenschaftssetzern ist veränderbar.
RubberDuck
1
@Kritner Wie werden die Eigenschaften als Primärschlüssel verwendet? Dies erfordert vermutlich eine gewisse Logik (Nullprüfungen) oder Regeln (z. B. hat NAME Vorrang vor AGE). Laut OOP gehört dieser Code zu dieser Klasse. Könnten Sie das als Argument verwenden?
user949300
2

Wie Sie mir sagten, Fooist ein Geschäftsobjekt. Es sollte ein gewisses Verhalten in Methoden entsprechend Ihrer Domain kapseln und seine Daten müssen so gekapselt wie möglich bleiben.

In dem gezeigten Beispiel Fooähnelt es eher einem DTO und verstößt gegen alle OOP-Prinzipien (siehe Anämisches Domänenmodell )!

Als Verbesserungen würde ich vorschlagen:

  • Machen Sie diese Klasse so unveränderlich wie möglich. Wie Sie sagten, möchten Sie einige Unit-Tests durchführen. Eine obligatorische Fähigkeit zum Testen von Einheiten ist der Determinismus , und Determinismus kann durch Unveränderlichkeit erreicht werden, da er die Nebenwirkungsprobleme löst .
  • Teilen Sie diese Gottklasse in mehrere Klassen auf, die jeweils nur eine Aufgabe ausführen (siehe SRP ). Unit-Test einer 30-Attribute-Klasse in wirklich einer PITA .
  • Entfernen Sie die Konstruktorredundanz, indem Sie nur einen Hauptkonstruktor und die anderen den Hauptkonstruktor aufrufen.
  • Entfernen Sie unnötige Getter. Ich bezweifle ernsthaft, dass alle Getter wirklich nützlich sind.
  • Bringen Sie Geschäftslogik in Geschäftsklassen zurück, hier gehört sie hin!

Dies könnte eine potenzielle überarbeitete Klasse mit folgenden Bemerkungen sein:

public sealed class Foo
{
    private readonly string field1;
    private readonly string field2;
    private readonly string field3;
    private readonly string field4;

    public Foo() : this("default1", "default2")
    { }

    public Foo(string in1, string in2) : this(in1, in2, "default3", "default4")
    { }

    public Foo(string in1, string in2, string in3, string in4)
    {
        field1 = in1;
        field2 = in2;
        field3 = in3;
        field4 = in4;
    }

    //Methods with business logic

    //Really needed ?
    public Prop1
    { get; }

    //Really needed ?
    public Prop2
    { get; }

    //Really needed ?
    public Prop3
    { get; }

    //Really needed ?
    public Prop4
    { get; }

    //Really needed ?
    public Prop5
    { get; }
}
Gepunktet
quelle
2

Wie man gegen diese „vollständig öffentliche“ Denkweise des Entwurfs von Geschäftsobjektklassen argumentiert

  • "Es gibt verschiedene Methoden, die in den Kundenklassen verstreut sind und mehr als nur DTO-Aufgaben erfüllen. Sie gehören zusammen."
  • "Es mag ein 'DTO' sein, aber es hat eine Geschäftsidentität - wir müssen Equals überschreiben."
  • "Wir müssen diese sortieren - wir müssen IComparable implementieren"
  • "Wir reihen mehrere dieser Eigenschaften aneinander. Lassen Sie uns ToString überschreiben, damit nicht jeder Client diesen Code schreiben muss."
  • "Wir haben mehrere Kunden, die dasselbe mit dieser Klasse machen. Wir müssen den Code austrocknen."
  • "Der Client manipuliert eine Sammlung dieser Dinge. Es ist offensichtlich, dass es sich um eine benutzerdefinierte Sammlungsklasse handeln sollte. Viele der früheren Punkte machen diese benutzerdefinierte Sammlung funktionaler als derzeit."
  • "Sehen Sie sich all die mühsamen, exponierten Manipulationen von Zeichenfolgen an! Wenn Sie diese in geschäftsrelevanten Methoden zusammenfassen, wird dies unsere Codierungsproduktivität steigern."
  • "Wenn Sie diese Methoden in der Klasse zusammenfassen, können Sie sie testen, da wir uns nicht mit der komplexeren Client-Klasse befassen müssen."
  • "Wir können diese überarbeiteten Methoden jetzt einem Komponententest unterziehen. Da der Client aus den Gründen x, y, z nicht testbar ist.

  • Soweit eines der oben genannten Argumente insgesamt vorgebracht werden kann:

    • Die Funktionalität wird wiederverwendbar.
    • Es ist trocken
    • Es ist von den Kunden entkoppelt.
    • Das Codieren gegen die Klasse ist schneller und weniger fehleranfällig.
  • "Eine gut gemachte Klasse verbirgt den Status und macht Funktionen verfügbar. Dies ist der Ausgangspunkt für das Entwerfen von OO-Klassen."
  • "Das Endergebnis bringt die Geschäftsdomäne, die unterschiedlichen Entitäten und ihre explizite Interaktion weitaus besser zum Ausdruck."
  • "Diese 'Overhead'-Funktionalität (dh keine expliziten funktionalen Anforderungen, aber es muss getan werden) fällt deutlich auf und folgt dem Prinzip der Einzelverantwortung.
Radarbob
quelle