Übergabe der Mitgliedsvariablen als Methodenparameter

In einem Projekt habe ich folgenden Code gefunden:

class SomeClass
{
    private SomeType _someField;

    public SomeType SomeField
    {
        get { return _someField; }
        set { _someField = value; }
    }

    protected virtual void SomeMethod(/*...., */SomeType someVar)
    {
    }

    private void SomeAnotherMethod()
    {
        //.............
        SomeMethod(_someField);
        //.............
    }

};

Wie kann ich meine Teamkollegen davon überzeugen, dass dies ein schlechter Code ist?

Ich halte das für eine unnötige Komplikation. Warum eine Mitgliedsvariable als Methodenparameter übergeben, wenn Sie bereits Zugriff darauf haben? Dies ist auch eine Verletzung der Kapselung.

Haben Sie weitere Probleme mit diesem Code?

Ich denke, dies ist ein gültiges Thema, aber der Grund, warum Sie gemischte Antworten erhalten, ist die Art und Weise, wie die Frage gebildet wurde. Persönlich habe ich die gleichen Erfahrungen mit meinem Team gemacht, bei denen es nicht erforderlich war, Mitglieder als Argumente zu übergeben, und den Code verschlungen habe. Wir hätten eine Klasse, die mit einer Reihe von Mitgliedern arbeitet, aber einige Funktionen greifen direkt auf Mitglieder zu, und andere Funktionen würden dieselben Mitglieder über Parameter ändern (dh völlig andere Namen verwenden), und es gab absolut keinen technischen Grund, dies zu tun. Aus technischen Gründen meine ich ein Beispiel, das Kate zur Verfügung gestellt hat.

Ich würde empfehlen, einen Schritt zurückzutreten und statt sich auf die Übergabe von Mitgliedern als Parameter zu konzentrieren, Diskussionen mit Ihrem Team über Klarheit und Lesbarkeit zu beginnen. Diskutieren Sie entweder formal oder nur in den Fluren, was das Lesen einiger Codesegmente und andere Codesegmente erschwert. Identifizieren Sie dann Qualitätsmaßstäbe oder Attribute für sauberen Code, nach denen Sie als Team streben würden. Selbst wenn wir an Projekten auf der grünen Wiese arbeiten, verbringen wir mehr als 90% der Zeit mit Lesen und sobald der Code geschrieben ist (sagen wir 10-15 Minuten später), geht er in die Wartung, wo die Lesbarkeit noch wichtiger ist.

Für Ihr spezielles Beispiel hier würde ich das Argument verwenden, dass weniger Code immer besser lesbar ist als mehr Code. Eine Funktion mit drei Parametern ist für das Gehirn schwerer zu verarbeiten als eine Funktion mit keinem oder einem Parameter. Wenn es einen anderen Variablennamen gibt, muss das Gehirn beim Lesen des Codes noch etwas anderes im Auge behalten. Wenn Sie sich also an "int m_value" und dann an "int localValue" erinnern und sich daran erinnern, dass das eine wirklich bedeutet, dass das andere für Ihr Gehirn immer teurer ist, als wenn Sie einfach mit "m_value" arbeiten.

Für mehr Munition und Ideen würde ich empfehlen, eine Kopie von Onkel Bobs Clean Code mitzunehmen .

Ich kann mir eine Rechtfertigung für die Übergabe eines Mitgliedsfeldes als Parameter in einer (privaten) Methode vorstellen: Sie macht deutlich, wovon Ihre Methode abhängt.

Wie Sie sagen, sind alle Mitgliedsfelder implizite Parameter Ihrer Methode, da das gesamte Objekt ist. Wird jedoch wirklich das gesamte Objekt benötigt, um das Ergebnis zu berechnen? Wenn SomeMethodes sich um eine interne Methode handelt, die nur davon abhängt _someField, ist es nicht sauberer, diese Abhängigkeit explizit zu machen? Wenn Sie diese Abhängigkeit explizit angeben, deuten Sie möglicherweise auch darauf hin, dass Sie diesen Codeabschnitt tatsächlich aus Ihrer Klasse heraus umgestalten können! (Ich nehme an, wir sprechen hier nicht über Getter oder Setter, sondern über Code, der tatsächlich etwas berechnet.)

Ich würde nicht dasselbe Argument für öffentliche Methoden vorbringen, da der Aufrufer weder weiß noch sich darum kümmert, welcher Teil des Objekts für die Berechnung des Ergebnisses relevant ist ...

Ich sehe einen starken Grund dafür, Mitgliedsvariablen als Funktionsargumente an private Methoden zu übergeben - Funktionsreinheit. Mitgliedsvariablen sind effektiv ein globaler Zustand aus der Sicht, und außerdem ein veränderlicher globaler Zustand, wenn sich das Mitglied während der Ausführung der Methode ändert. Durch Ersetzen von Membervariablenreferenzen durch Methodenparameter können wir eine Funktion effektiv rein machen. Reine Funktionen sind nicht von einem externen Zustand abhängig und haben keine Nebenwirkungen. Sie liefern bei gleichen Eingabeparametern immer die gleichen Ergebnisse. Dies erleichtert das Testen und die zukünftige Wartung.

Sicher ist es weder einfach noch praktisch, alle Ihre Methoden als reine Methoden in einer OOP-Sprache zu haben. Aber ich glaube, Sie gewinnen viel an Code-Klarheit, wenn Sie die Methoden, die komplexe Logik handhaben, rein halten und unveränderliche Variablen verwenden, während Sie die unreine "globale" Zustandsbehandlung innerhalb dedizierter Methoden getrennt halten.

Die Übergabe von Membervariablen an eine öffentliche Funktion desselben Objekts, wenn die Funktion extern aufgerufen wird, würde meiner Meinung nach jedoch einen großen Codegeruch ausmachen.

Wenn die Funktion mehrmals aufgerufen wird, manchmal diese Mitgliedsvariable übergeben wird und manchmal etwas anderes übergeben wird, ist es OK. Zum Beispiel würde ich das überhaupt nicht für schlecht halten:

if ( CalculateCharges(newStartDate) > CalculateCharges(m_StartDate) )
{
     //handle increase in charges
}

Dabei newStartDatehandelt es sich um eine lokale Variable und m_StartDateeine Mitgliedsvariable.

Wenn die Funktion jedoch immer nur mit der übergebenen Mitgliedsvariablen aufgerufen wird, ist das seltsam. Mitgliedsfunktionen arbeiten die ganze Zeit mit Mitgliedsvariablen. Dies geschieht möglicherweise (abhängig von der Sprache, in der Sie arbeiten), um eine Kopie der Mitgliedsvariablen zu erhalten. Wenn dies der Fall ist und Sie dies nicht sehen können, ist der Code möglicherweise besser, wenn der gesamte Prozess dadurch explizit dargestellt wird.

Was niemand angerührt hat, ist, dass SomeMethod virtuell geschützt ist. Dies bedeutet, dass eine abgeleitete Klasse sie verwenden UND ihre Funktionalität erneut implementieren kann. Eine abgeleitete Klasse hätte keinen Zugriff auf die private Variable und könnte daher keine benutzerdefinierte Implementierung von SomeMethod bereitstellen, die von der privaten Variablen abhängt. Anstatt die Abhängigkeit von der privaten Variablen zu übernehmen, muss der Aufrufer diese Deklaration übergeben.

GUI-Frameworks verfügen normalerweise über eine Art 'View'-Klasse, die auf dem Bildschirm gezeichnete Objekte darstellt, und diese Klasse bietet normalerweise eine Methode, invalidateRect(Rect r)mit der ein Teil des Zeichenbereichs als neu zu zeichnend markiert wird. Clients können diese Methode aufrufen, um eine Aktualisierung eines Teils der Ansicht anzufordern. Eine Ansicht kann jedoch auch eine eigene Methode aufrufen, z.

invalidateRect(m_frame);

um den gesamten Bereich neu zu zeichnen. Dies kann beispielsweise der Fall sein, wenn es zum ersten Mal zur Ansichtshierarchie hinzugefügt wird.

Daran ist nichts auszusetzen - der Rahmen der Ansicht ist ein gültiges Rechteck, und die Ansicht selbst weiß, dass sie sich selbst neu zeichnen möchte. Die View-Klasse könnte eine separate Methode bereitstellen, die keine Parameter akzeptiert und stattdessen den Frame der Ansicht verwendet:

invalidateFrame();

Aber warum sollte man dafür eine spezielle Methode hinzufügen, wenn man die allgemeinere verwenden kann invalidateRect()? Wenn Sie sich für die Bereitstellung entschieden haben invalidateFrame(), würden Sie sie höchstwahrscheinlich allgemeiner implementieren invalidateRect():

View::invalidateFrame(void)
{
    invalidateRect(m_frame)
}

Warum Variable als Methodenparameter übergeben, wenn Sie bereits Zugriff darauf haben?

Sie sollten Instanzvariablen als Parameter an Ihre eigene Methode übergeben, wenn die Methode nicht speziell für diese Instanzvariable ausgeführt wird. Im obigen Beispiel ist der Rahmen der Ansicht für die invalidateRect()Methode nur ein weiteres Rechteck .

Dies ist sinnvoll, wenn die Methode eine Utility-Methode ist. Angenommen, Sie müssen einen eindeutigen Kurznamen aus mehreren freien Textzeichenfolgen ableiten.

Sie möchten nicht für jede Zeichenfolge eine eigene Implementierung programmieren, sondern die Zeichenfolge an eine gemeinsame Methode übergeben.

Wenn die Methode jedoch immer für ein einzelnes Element ausgeführt wird, ist es etwas dumm, sie als Parameter zu übergeben.

Der Hauptgrund dafür, dass eine Membervariable in einer Klasse enthalten ist, besteht darin, dass Sie sie an einem Ort festlegen können und ihren Wert für alle anderen Methoden in der Klasse verfügbar haben. Im Allgemeinen ist daher zu erwarten, dass die Membervariable nicht an eine Methode der Klasse übergeben werden muss.

Ich kann mir jedoch einige Gründe vorstellen, warum Sie die Member-Variable möglicherweise an eine andere Methode der Klasse übergeben möchten. Die erste ist, wenn Sie sicherstellen müssen, dass der Wert der Mitgliedsvariablen bei Verwendung mit der aufgerufenen Methode unverändert verwendet werden muss, auch wenn diese Methode den tatsächlichen Mitgliedsvariablenwert zu einem bestimmten Zeitpunkt im Prozess ändern muss. Der zweite Grund hängt mit dem ersten zusammen, da Sie möglicherweise die Unveränderlichkeit eines Werts im Rahmen einer Methodenkette gewährleisten möchten - beispielsweise bei der Implementierung einer fließenden Syntax.

In Anbetracht dessen würde ich nicht sagen, dass der Code als solcher "schlecht" ist, wenn Sie eine Mitgliedsvariable an eine der Klassenmethoden übergeben. Ich würde jedoch vorschlagen, dass es im Allgemeinen nicht ideal ist, da es eine Menge Code-Duplizierung fördern kann, wenn der Parameter beispielsweise einer lokalen Variablen zugewiesen wird, und die zusätzlichen Parameter dem Code "Rauschen" hinzufügen, wenn er nicht benötigt wird. Wenn Sie ein Fan des Buches "Bereinigter Code" sind, sollten Sie die Anzahl der Methodenparameter auf ein Minimum beschränken, und zwar nur dann, wenn es keine sinnvollere Möglichkeit gibt, mit der Methode auf den Parameter zuzugreifen .

Einige Dinge, die mir einfallen, wenn ich darüber nachdenke:

1. Im Allgemeinen sind Methodensignaturen mit weniger Parametern leichter zu verstehen. Ein wichtiger Grund, warum Methoden erfunden wurden, bestand darin, lange Parameterlisten zu eliminieren, indem sie mit den Daten verknüpft wurden, mit denen sie arbeiten.
2. Wenn Sie die Methodensignatur von der Mitgliedsvariablen abhängig machen, wird es in Zukunft schwieriger, diese Variablen zu ändern, da Sie nicht nur innerhalb der Methode Änderungen vornehmen müssen, sondern auch überall dort, wo die Methode aufgerufen wird. Und da SomeMethod in Ihrem Beispiel geschützt ist, müssen auch Unterklassen geändert werden.
3. Methoden (öffentlich oder privat), die nicht von den Klasseninternalen abhängen, müssen sich nicht in dieser Klasse befinden. Sie könnten in Nutzenmethoden zerlegt werden und genauso glücklich sein. Sie haben so gut wie nichts damit zu tun, Teil dieser Klasse zu sein. Wenn keine andere Methode von dieser Variablen abhängt, nachdem Sie die Methode (n) verschoben haben, sollte diese Variable auch verwendet werden! Höchstwahrscheinlich sollte sich die Variable auf einem eigenen Objekt befinden, wobei diese Methode oder Methoden, die damit arbeiten, öffentlich werden und von der übergeordneten Klasse zusammengesetzt werden.
4. Das Weitergeben von Daten an verschiedene Funktionen wie Ihre Klasse ist ein prozedurales Programm mit globalen Variablen, die dem OO-Design nur zuwiderlaufen. Dies ist nicht die Absicht von Membervariablen und Memberfunktionen (siehe oben), und es hört sich so an, als ob Ihre Klasse nicht sehr zusammenhängend ist. Ich denke, es ist ein Codegeruch, der eine bessere Methode zum Gruppieren Ihrer Daten und Methoden vorschlägt.

Sie fehlen, wenn der Parameter ("Argument") referenziert oder schreibgeschützt ist.

class SomeClass
{
    protected SomeType _someField;
    public SomeType SomeField
    {
        get { return _someField; }

        set {
          if (doSomeValidation(value))
          {
            _someField = value;
          }
        }
    }

    protected virtual void ModifyMethod(/*...., */ ref SomeType someVar)
    { 
      // ...
    }    

    protected virtual void ReadMethod(/*...., */ SomeType someVar)
    { 
      // ...
    }

    private void SomeAnotherMethod()
    {
        //.............

        // not recommended, but, may be required in some cases
        ModifyMethod(ref this._someField);

        //.............

        // recommended, but, verbose
        SomeType SomeVar = this.someField;
        ModifyMethod(ref SomeVar);
        this.someField = SomeVar;

        //.............

        ReadMethod(this.someField);
        //.............
    }

};

Viele Entwickler weisen in der Regel die internen Felder einer Variablen in den Konstruktormethoden direkt zu. Es gibt einige Ausnahmen.

Denken Sie daran, dass "Setter" zusätzliche Methoden haben können, nicht nur Zuweisungen, und manchmal sogar virtuelle Methoden sein oder virtuelle Methoden aufrufen können.

Hinweis: Ich empfehle, die internen Felder der Eigenschaften als "geschützt" zu belassen.