Violazione del principio di responsabilità singola?

Lo guarderei in questo modo:

Un metodo dovrebbe chiamare altri metodi (nello stesso o in altri oggetti) che lo rende un metodo composto o fare "calcoli primitivi" (stesso livello di astrazione).

La responsabilità di un metodo composto è "chiama altri metodi".

Quindi se il tuo metodo Save esegue alcuni "calcoli primitivi" (ad esempio verifica i valori di ritorno), l'SPR potrebbe essere violato. Se questo "calcolo primitivo" risiede in un altro metodo chiamato dal tuo metodo Save SRP non viene violato.

La versione aggiornata del metodo Save non segue il principio livello di astrazione singola . Poiché questo è il problema più importante, dovresti estrarlo in un nuovo metodo.

Converte Save in un metodo composto . Come scritto, la responsabilità di un metodo composto è "chiama altri metodi". Pertanto chiamare UpdateGroupBilling([Parameters]) qui non è una violazione dell'SRP, ma una decisione del business case.

La singola responsabilità può essere interpretata come la funzione / classe dovrebbe avere solo un motivo per cambiare.

Quindi il tuo attuale metodo Save violerà tale principio, perché dovrebbe essere modificato da più di un motivo.
 1. Salva logica modificata
 2. Se decidi di aggiornare qualcos'altro oltre all'aggiornamento del gruppo di fatturazione

Puoi refactoring introducendo il metodo SaveModel che sarà responsabile solo del salvataggio. E introducendo un altro metodo che combina tutte le operazioni necessarie in base alle tue esigenze. Quindi finisci con due metodi

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

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

Dove il metodo SaveModel avrà la responsabilità di salvare il modello nel database e una ragione per cambiare - se il salvataggio della "logica" cambierà.

Save ora ha la responsabilità di chiamare tutti i metodi richiesti per un processo di "salvataggio" completo e ha un motivo per cambiare - se la quantità del metodo richiesto cambierà.

Penso che la validazione possa essere spostata anche al di fuori di SaveModel .

La singola responsabilità è IMHO non facile da definire.

Una semplice regola empirica è:

When I have to explain, what a method/class does, and have to use the word »and«, it is an indicator, that something smelly may be going on.

Da un lato, è solo un indicatore e dall'altro lato funziona meglio all'inverso: se si verificano due cose, non si può evitare di usare la parola »e« e poiché si stanno facendo due cose, si viola SRP .

Ma, d'altra parte, vuol dire che se fai più di una cosa, stai violando SRP ? No. Perché altrimenti eri limitato al codice banale e ai problemi banali da risolvere. Ti faresti male se fossi troppo severo nell'interpretazione.

Un'altra prospettiva su SRP è: un livello di astrazione . Finché hai a che fare con un livello di astrazione, stai per lo più bene.

Che cosa significa tutto questo per la tua domanda:

I believe UpdateGroupBilling should not be called inside the save method as it violates the single responsibility principle. But, he says that each time a payment is made, the billing should be updated. Hence this is the correct approach.

Per decidere se questa è una violazione di SRP , è necessario sapere cosa succede in save() -Method. Se il metodo è -come il nome- suggerisce di mantenere il modello nel database, una chiamata a UpdateGroupBilling è IMHO una violazione di SRP , poiché stai estendendo il contesto di »salvare un pagamento«. Lo parafraserai con »Sto salvando il pagamento e aggiorno la fatturazione di gruppo«, che è - come ho detto prima - un indicatore (possibile) di violazione di SRP .

D'altra parte, se il metodo sta descrivendo una "ricetta di pagamento" -i.e. quali sono i passaggi da prendere nel processo e decidere: ogni volta che un pagamento è finito, deve essere salvato (gestito in un altro posto) e quindi i fatturati di gruppo devono essere aggiornati (trattati in un altro luogo), che sarebbe nessuna violazione di SRP, dal momento che non si lascia l'astrazione della "ricetta": si distingue tra cosa fare (nella ricetta) e dove è fatto (nella classe / metodo / modulo). Ma se questo è ciò che fa il tuo save() -method (descrivendo quali passi devi prendere) dovresti rinominarlo.

Senza ulteriore contesto, è difficile dire qualcosa di concreto.

Modifica: hai aggiornato il tuo post inziale. Direi che questo metodo viola il SRP e dovrebbe essere refactored. Il recupero dei dati dovrebbe essere scomposto (dovrebbe essere un parametro di questo metodo). L'aggiunta di di dati (updateBy / On) dovrebbe essere eseguita altrove. Il salvataggio dovrebbe essere fatto altrove. Quindi sarebbe OK lasciare UpdateGroupBilling([Parameters]) così com'è.

Difficile essere sicuro senza un contesto completo, ma penso che la tua intuizione sia corretta. Il mio indicatore SRP preferito personale è se stai per sapere esattamente dove andare e cambiare qualcosa per modificare una funzionalità mesi dopo.

Una classe che definisce un tipo di pagamento non è dove mi aspetterei di andare a ridefinire chi paga o come viene effettuato il pagamento. Mi aspetto che sia uno dei tanti tipi di pagamento che fornisce semplicemente dettagli critici che alcune altre parti dell'app utilizzano per avviare, condurre o eseguire il rollback / annullare una transazione, raccogliendo tali dettagli attraverso un'interfaccia uniforme.

Sembra anche una ricetta per una violazione del principio di DRY più generale, se hai intenzione di avere più tipi ciascuno che ordina le proprie transazioni usando molti degli stessi metodi. SOLID non si sente sempre al 100% rilevante per lo sviluppatore JavaScript (ho il sospetto che molte siano spiegate male), ma DRY è lo standard d'oro nella metodologia di programmazione generale IMO. Ogni volta che mi imbatto in un'idea che sembra un'estensione di DRY, la prendo in considerazione. SRP è uno di quelli. Se tutti rimangono in argomento, è meno probabile che tu sia tentato di ripeterti.

Se c'è concettualmente solo un modo per UpdateGroupBilling(...) , e accade sempre solo in quel posto, allora probabilmente va bene dov'è. Ma la domanda è legata al concetto, non alle circostanze temporali, cioè "per ora lo facciamo solo qui".

Se non è il caso, quindi un modo per refactoring sarebbe utilizzare Publish / Subscribe e notificare ogni volta che il pagamento viene salvato, e fare in modo che la fatturazione sottoscriva quell'evento e aggiorni le voci pertinenti. Ciò è utile soprattutto se è necessario disporre di diversi tipi di fatturazione che devono essere aggiornati. Un altro metodo potrebbe essere quello di pensare alla fatturazione come un modello strategico, cioè sceglierne uno e usarlo, o accettarlo come un parametro. Oppure, se la fatturazione non avviene sempre, puoi aggiungerla come decoratore, ecc. Ecc. Ma di nuovo, questo dipende dal tuo modello concettuale e da come vuoi pensarci, quindi dovresti capirlo .

Una cosa importante da considerare, però, è la gestione degli errori. Se la fatturazione fallisce, cosa succede con le operazioni precedenti?

Penso che questo design stia violando lo SRP, ma è molto semplice correggere e per fare ancora tutto il resto necessario.

Pensaci ai messaggi e a cosa sta succedendo in questo metodo. Dovrebbe essere salvato GroupBillingPayment , ma non c'è niente di sbagliato se GroupBillingPayment notifica classi che è stato salvato. Soprattutto se sta implementando un pattern che espone esplicitamente questo comportamento.

Potresti utilizzare il osservatore modello.

Ecco un esempio di come funzionerebbe nel tuo caso:

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);
}

Ora tutto ciò che devi fare è creare uno o più Observers che verrà associato a GroupBillingPayment e ricevere così una notifica ogni volta che viene salvato. Mantiene le proprie dipendenze, non sa cosa viene notificato, quindi non dipende affatto da questi.

Vuoi raggiungere X. A meno che X non sia abbastanza banale, scrivi un metodo che farà di tutto per ottenere X, chiama il metodo "achieveX" e chiama quel metodo. La sola responsabilità di "achieveX" è fare tutto per ottenere X. "achieveX" non dovrebbe fare altro.

Se X è complesso, molte azioni potrebbero essere necessarie per ottenere X. L'insieme di azioni necessarie potrebbe cambiare nel tempo, quindi quando ciò accade, cambi il metodo achieveX, ma se tutto va bene, puoi comunque chiamare esso.

Nel tuo esempio, il metodo "Salva" è responsabilità di non salvare la bolletta e aggiornare la fatturazione di gruppo (due responsabilità), la sua responsabilità è di prendere tutte le azioni necessarie per rendere la fattura registrata in modo permanente. Una responsabilità. Forse l'hai chiamato male.

Se capisco il tuo punto, avrei il pagamento per sollevare un evento come "Pagamento effettuato" e la classe che gestisce la fatturazione sottoscrivendola. In questo modo il gestore dei pagamenti non sa nulla della fatturazione.