Verstoß gegen das Prinzip der Einzelverantwortung?

Ich habe kürzlich eine Debatte mit einem anderen Entwickler über die folgende Klasse geführt:

public class GroupBillingPayment
{
    public void Save(IGroupBillingPayment model)
    {
        if (model == null || UserInfo.UserID == 0)
        {
            throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
        }

        Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
        Mapper.Map(model, groupBillingPayment);
        ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
        groupBillingPayment.UpdatedBy = UserInfo.UserID;
        groupBillingPayment.UpdatedOn = DateTime.Now;
        RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
        UpdateGroupBilling([Parameters])
    }
}

Ich glaube, UpdateGroupBillingsollte nicht innerhalb der Speichermethode aufgerufen werden, da dies gegen das Prinzip der Einzelverantwortung verstößt. Er sagt jedoch, dass jedes Mal, wenn eine Zahlung erfolgt, die Abrechnung aktualisiert werden sollte. Daher ist dies der richtige Ansatz.

Meine Frage, wird SRP hier verletzt? Wenn ja, wie können wir es besser umgestalten, damit beide Kriterien erfüllt werden?

Ich würde es so sehen:

Eine Methode sollte entweder andere Methoden (in denselben oder anderen Objekten) aufrufen, wodurch sie zu einer zusammengesetzten Methode wird, oder "primitive Berechnungen" durchführen (dieselbe Abstraktionsebene).

Die Verantwortung einer zusammengesetzten Methode liegt darin, "andere Methoden aufzurufen".

Wenn Ihre SaveMethode also selbst einige "primitive Berechnungen" durchführt (z. B. Rückgabewerte überprüft), kann die SPR verletzt werden. Wenn diese "primitiven Berechnungen" in einer anderen von Ihrer SaveMethode aufgerufenen Methode leben, wird SRP nicht verletzt.

Die aktualisierte Version der SaveMethode folgt nicht dem Prinzip der einzelnen Abstraktionsschicht . Da dies das wichtigere Problem ist, sollten Sie dies in eine neue Methode extrahieren.

Dies wird Savein eine zusammengesetzte Methode umgewandelt . Wie geschrieben, liegt die Verantwortung einer zusammengesetzten Methode darin, "andere Methoden aufzurufen". Daher ist das Anrufen UpdateGroupBilling([Parameters])hier kein Verstoß gegen die SRP, sondern eine Business-Case-Entscheidung.

Einzelverantwortung kann so interpretiert werden, dass Funktion / Klasse nur einen Grund zur Änderung haben sollte.

Ihre derzeitige SaveMethode verstößt also gegen dieses Prinzip, da sie aus mehr als einem Grund geändert werden sollte.
1. Speicherlogik geändert
2. Wenn Sie zusätzlich zur Aktualisierung der Abrechnungsgruppe noch etwas anderes aktualisieren möchten

Sie können es umgestalten, indem Sie eine SaveModelMethode einführen , die nur für das Speichern verantwortlich ist. Und Einführung einer weiteren Methode, die alle erforderlichen Vorgänge basierend auf Ihren Anforderungen kombiniert. Sie haben also zwei Methoden

public void SaveModel(IGroupBillingPayment model)
{
    // only saves model
}

public void Save(IGroupBillingPayment model)
{
    SaveModel(model);
    UpdateGroupBilling([Parameters]);
}

Wo die SaveModelMethode für das Speichern des Modells in der Datenbank verantwortlich ist und ein Grund für die Änderung - wenn sich das Speichern der "Logik" ändert.

Save Die Methode hat nun die Verantwortung, alle erforderlichen Methoden für den vollständigen "Speicher" -Prozess aufzurufen, und hat einen Grund zur Änderung - wenn sich die Menge der erforderlichen Methode ändert.

Ich denke, die Validierung kann auch außerhalb von verschoben SaveModelwerden.

Einzelverantwortung ist meiner Meinung nach ein nicht einfach zu nagelndes Konzept.

Eine einfache Faustregel lautet:

Wenn ich erklären muss, was eine Methode / Klasse tut und das Wort »und« verwenden muss, ist dies ein Indikator dafür, dass möglicherweise etwas stinkt .

Einerseits ist es nur ein Indikator und andererseits funktioniert es besser umgekehrt: Wenn zwei Dinge vor sich gehen, können Sie die Verwendung des Wortes »und« nicht vermeiden, und da Sie zwei Dinge tun, verletzen Sie die SRP .

Aber heißt das andererseits, wenn Sie mehr als eine Sache tun, verletzen Sie die SRP ? Nein, weil Sie sonst auf trivialen Code und triviale Probleme beschränkt waren, die es zu lösen galt. Sie würden sich verletzen, wenn Sie in der Interpretation zu streng wären.

Eine andere Perspektive auf SRP ist: eine Abstraktionsebene . Solange Sie sich mit einer Abstraktionsebene befassen, geht es Ihnen meistens gut.

Was bedeutet das alles für Ihre Frage:

Ich bin der Meinung, dass UpdateGroupBilling nicht innerhalb der Speichermethode aufgerufen werden sollte, da dies gegen das Prinzip der Einzelverantwortung verstößt. Er sagt jedoch, dass jedes Mal, wenn eine Zahlung erfolgt, die Abrechnung aktualisiert werden sollte. Daher ist dies der richtige Ansatz.

Um zu entscheiden, ob dies eine Verletzung von SRP ist, muss man wissen, was in der save()Methode vor sich geht. Wenn die Methode ist -wie die Namen- das Modell zu beharren auf die Datenbank verantwortlich schlägt vor, auf einen Anruf UpdateGroupBilling ist IMHO eine Verletzung der SRP , da Sie den Kontext »erweitern eine Zahlung Speichern«. Sie würden es mit »Ich speichere die Zahlung und aktualisiere die Gruppenabrechnung« umschreiben, was - wie ich bereits sagte - ein (möglicher) Indikator für einen Verstoß gegen SRP ist .

Wenn die Methode andererseits ein "Zahlungsrezept" beschreibt - dh welche Schritte im Prozess zu unternehmen sind - und entscheidet: Jedes Mal, wenn eine Zahlung abgeschlossen ist, muss sie gespeichert (an einer anderen Stelle behandelt) und dann Die Gruppenabrechnungen müssen aktualisiert werden (an einer anderen Stelle behandelt werden). Dies wäre kein Verstoß gegen SRP, da Sie die Abstraktion des "Rezepts" nicht verlassen: Sie unterscheiden zwischen dem, was zu tun ist (im Rezept) und wo es wird gemacht (in der entsprechenden Klasse / Methode / Modul). Wenn dies jedoch Ihre save()Methode ist (die beschreibt, welche Schritte ausgeführt werden sollen), sollten Sie sie umbenennen.

Ohne weiteren Kontext ist es schwer, etwas Konkretes zu sagen.

Bearbeiten: Sie haben Ihren ersten Beitrag aktualisiert. Ich würde sagen, dass diese Methode die SRP verletzt und überarbeitet werden sollte. Das Abrufen der Daten sollte herausgerechnet werden (es sollte ein Parameter dieser Methode sein). Das Hinzufügen von Daten (updateBy / On) sollte an anderer Stelle erfolgen. Das Speichern sollte an anderer Stelle erfolgen. Dann wäre es in Ordnung, so zu bleiben, UpdateGroupBilling([Parameters])wie es ist.

Ohne vollständigen Kontext ist es schwer, sicher zu sein, aber ich denke, Ihre Intuition ist richtig. Mein persönlicher Lieblings-SRP-Indikator ist, ob Sie genau wissen, wohin Sie gehen und etwas ändern müssen, um eine Funktion Monate später zu ändern.

In einer Klasse, die eine Zahlungsart definiert, würde ich nicht erwarten, neu zu definieren, wer die Zahlung vornimmt oder wie die Zahlung erfolgt. Ich würde erwarten, dass es sich um eine von vielen Zahlungsarten handelt, die lediglich wichtige Details bereitstellen, die ein anderer Teil der App verwendet, um eine Transaktion zu initiieren, durchzuführen oder zurückzusetzen / abzubrechen und diese Details über eine einheitliche Oberfläche zu erfassen.

Es sieht auch nach einem Rezept für eine allgemeinere Verletzung des DRY-Prinzips aus, wenn mehrere Typen ihre eigenen Transaktionen mit vielen der gleichen Methoden sortieren. SOLID ist nicht immer zu 100% relevant für hauptsächlich JavaScript-Entwickler (ich vermute, dass viele davon schlecht erklärt wurden), aber DRY ist der Goldstandard in der allgemeinen Programmiermethode IMO. Jedes Mal, wenn ich auf eine Idee stoße, die sich wie eine Erweiterung von DRY anfühlt, denke ich darüber nach. SRP ist eine davon. Wenn alle beim Thema bleiben, sind Sie weniger versucht, sich zu wiederholen.

Wenn es konzeptionell nur einen Weg UpdateGroupBilling(...)gibt und es immer nur an diesem Ort passiert, dann ist es wahrscheinlich in Ordnung, wo es ist. Die Frage bezieht sich jedoch auf das Konzept, nicht auf zeitliche Umstände, dh "vorerst tun wir es nur hier".

Wenn beides nicht der Fall ist, besteht eine Möglichkeit zur Umgestaltung darin, Publish / Subscribe zu verwenden und zu benachrichtigen, wann immer die Zahlung gespeichert wird, und die Abrechnung dieses Ereignis abonnieren zu lassen und die relevanten Einträge zu aktualisieren. Dies ist besonders dann gut, wenn Sie mehrere Arten von Abrechnungen benötigen, die aktualisiert werden müssen. Eine andere Methode wäre, sich die Abrechnung als Strategiemuster vorzustellen, dh eines auszuwählen und zu verwenden oder es sowie einen Parameter zu akzeptieren. Wenn die Abrechnung nicht immer erfolgt, können Sie sie als Dekorateur usw. hinzufügen. Dies hängt jedoch auch von Ihrem konzeptionellen Modell ab und davon, wie Sie darüber nachdenken möchten. Sie müssten es also herausfinden .

Eine wichtige Sache, die berücksichtigt werden muss, ist jedoch die Fehlerbehandlung. Was passiert mit den vorherigen Vorgängen, wenn die Abrechnung fehlschlägt?

Ich denke, dieses Design verstößt gegen die SRP, aber es ist wirklich einfach zu reparieren und trotzdem alles andere zu tun, was benötigt wird.

Denken Sie an Nachrichten und daran, was bei dieser Methode passiert. Es sollte das speichern GroupBillingPayment, aber es ist nichts falsch, wenn Klassen GroupBillingPayment benachrichtigt werden, dass es gespeichert wurde. Insbesondere, wenn ein Muster implementiert wird, das dieses Verhalten explizit aufdeckt.

Sie können das Observer- Muster verwenden.

Hier ist ein Beispiel, wie es in Ihrem Fall funktionieren würde:

public interface Subject {

    public void register(Observer observer);
    public void unregister(Observer observer);

    public void notifyObservers();      
}

public class GroupBillingPayment implements Subject {

    private List<Observer> observers;
    public void Save(IGroupBillingPayment model)
    {
        if (model == null || UserInfo.UserID == 0)
        {
            throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
        }

        Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
        Mapper.Map(model, groupBillingPayment);
        ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
        groupBillingPayment.UpdatedBy = UserInfo.UserID;
        groupBillingPayment.UpdatedOn = DateTime.Now;

        RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
        //UpdateGroupBilling([Parameters]) The Observer will have this responsability instead now
        notifyObservers();
    }

    public void notifyObservers() {
        for (Observer obj : observers) {
            obj.update();
        }
    }
}

public interface Observer {     
    public void update();

    public void setSubject(Subject sub);
}

Jetzt müssen Sie nur noch eine oder mehrere erstellen Observers, an die gebunden wird, GroupBillingPaymentund werden benachrichtigt, sobald sie gespeichert werden. Es behält seine eigenen Abhängigkeiten bei, weiß nicht, was benachrichtigt wird, und hängt daher überhaupt nicht davon ab.

Sie möchten X erreichen. Wenn X nicht ganz trivial ist, schreiben Sie eine Methode, die alles unternimmt, um X zu erreichen, rufen Sie die Methode "aimX" auf und rufen Sie diese Methode auf. Die einzige Verantwortung von "chieveX "besteht darin, alles zu tun, um X zu erreichen." AchieveX "sollte keine anderen Dinge tun.

Wenn X komplex ist, sind möglicherweise viele Aktionen erforderlich, um X zu erreichen. Die Anzahl der erforderlichen Aktionen kann sich im Laufe der Zeit ändern. Wenn dies geschieht, ändern Sie die Methode removeX. Wenn jedoch alles gut läuft, können Sie sie trotzdem einfach aufrufen.

In Ihrem Beispiel besteht die Aufgabe der Methode "Speichern" nicht darin, die Rechnung zu speichern und die Gruppenabrechnung zu aktualisieren (zwei Verantwortlichkeiten). Sie ist dafür verantwortlich, alle erforderlichen Maßnahmen zu ergreifen, um die Rechnung dauerhaft zu erfassen. Eine Verantwortung. Vielleicht hast du es schlecht genannt.

Wenn ich Ihren Standpunkt verstehe, hätte ich die Zahlung, um ein Ereignis wie "Zahlung geleistet" auszulösen, und die Klasse, die die Abrechnung abwickelt, die es abonniert. Auf diese Weise weiß der Zahlungsabwickler nichts über die Abrechnung.