Violação de princípio de responsabilidade única?

Recentemente, entrei em um debate com outro desenvolvedor sobre a classe abaixo:

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

Eu acredito UpdateGroupBillingque não deve ser chamado dentro do método save, pois viola o princípio da responsabilidade única. Porém, ele diz que sempre que um pagamento é feito, o faturamento deve ser atualizado. Portanto, esta é a abordagem correta.

Minha pergunta, o SRP está sendo violado aqui? Se sim, como podemos refatorá-lo melhor para que ambos os nossos critérios sejam atendidos?

Gostaria de olhar desta forma:

Um método deve chamar outros métodos (no mesmo ou em outros objetos), o que o torna um método composto ou fazer "cálculos primitivos" (mesmo nível de abstração).

A responsabilidade de um método composto é "chamar outros métodos".

Portanto, se o seu Savemétodo fizer alguns "cálculos primitivos" (por exemplo, verifica os valores de retorno), o SPR pode ser violado. Se esses "cálculos primitivos" residirem em outro método chamado pelo seu Savemétodo, o SRP não será violado.

A versão atualizada do Savemétodo não segue o princípio da camada de abstração única . Como esse é o problema mais importante, você deve extraí-lo para um novo método.

Isso será convertido Saveem um método composto . Como está escrito, a responsabilidade de um método composto é "chamar outros métodos". Portanto, ligar UpdateGroupBilling([Parameters])aqui não é uma violação do SRP, mas uma decisão de caso de negócios.

A responsabilidade única pode ser interpretada como função / classe deve ter apenas um motivo para mudar.

Portanto, seu Savemétodo atual violará esse princípio, porque deve ser alterado por mais de um motivo.
1. Salvar lógica alterada
2. Se você decidir atualizar outra coisa além de atualizar o grupo de cobrança

Você pode refatorá-lo, introduzindo o SaveModelmétodo que será responsável apenas pela economia. E introduzindo outro método que combina todas as operações necessárias com base em seus requisitos. Então você acaba com dois métodos

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

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

Onde o SaveModelmétodo terá a responsabilidade de salvar o modelo no banco de dados e um motivo para alterar - se salvar a "lógica" será alterada.

Save O método agora tem a responsabilidade de chamar todos os métodos necessários para o processo completo de "salvamento" e tem um motivo para alterar - se a quantidade do método necessário for alterada.

Eu acho que a validação também pode ser movida para fora SaveModel.

A responsabilidade única é IMHO, um conceito que não é fácil de definir.

Uma regra simples é:

Quando tenho que explicar, o que um método / classe faz e preciso usar a palavra »e«, é um indicador de que algo fedorento pode estar acontecendo .

Por um lado, é apenas um indicador e, por outro lado, funciona melhor ao contrário: se duas coisas estão acontecendo, você não pode evitar o uso da palavra »e« e, como você está fazendo duas coisas, viola o SRP .

Mas, por outro lado, isso significa que, se você fizer mais de uma coisa, estará violando o SRP ? Não. Porque, caso contrário, você estava limitado a códigos triviais e problemas triviais a serem resolvidos. Você se machucaria se fosse muito rigoroso na interpretação.

Outra perspectiva sobre o SRP é: um nível de abstração . Enquanto você estiver lidando com um nível de abstração, estará bem.

O que tudo isso significa para a sua pergunta:

Acredito que UpdateGroupBilling não deve ser chamado dentro do método save, pois viola o princípio de responsabilidade única. Porém, ele diz que sempre que um pagamento é feito, o faturamento deve ser atualizado. Portanto, esta é a abordagem correta.

Para decidir se isso é uma violação do SRP , é necessário saber o que está acontecendo no save()método. Se o método for, como o nome, sugere responsável por manter o modelo no banco de dados, uma chamada para IMHO UpdateGroupBilling é uma violação do SRP , pois você está estendendo o contexto de »salvar um pagamento«. Você deve parafrasear com »Estou salvando o pagamento e atualizando o faturamento do grupo«, que é - como eu disse anteriormente - um (possível) indicador de violação do SRP .

Por outro lado, se o método está descrevendo uma "receita de pagamento" - quais são as etapas a serem seguidas no processo - e decide: sempre que um pagamento é concluído, ele deve ser salvo (tratado em outro local) e depois o faturamento do grupo deve ser atualizado (tratado em outro local), isso não viola o SRP, pois você não está deixando a abstração da "receita": você distingue entre o que fazer (na receita) e onde é feito (na classe / método / módulo de acordo). Mas se é isso que seu save()método-faz (descrevendo quais etapas devem ser tomadas), você deve renomeá-lo.

Sem outro contexto, é difícil dizer algo concreto.

Editar: você atualizou sua postagem inicial. Eu diria que esse método viola o SRP e deve ser refatorado. A busca dos dados deve ser fatorada (deve ser um parâmetro desse método). A adição de dados (updateBy / On) deve ser feita em outro lugar. A economia deve ser feita em outro lugar. Seria bom sair UpdateGroupBilling([Parameters])como está.

Difícil ter certeza, sem contexto completo, mas acho que sua intuição está correta. Meu indicador SRP favorito é se você saberá exatamente para onde ir e alterar algo para modificar um recurso meses depois.

Uma classe que define um tipo de pagamento não é onde eu esperaria redefinir quem paga ou como é feito. Eu esperava que fosse um dos muitos tipos de pagamento que simplesmente fornece detalhes críticos que alguma outra parte do aplicativo usa para iniciar, conduzir ou reverter / cancelar uma transação, reunindo esses detalhes por meio de uma interface uniforme.

Também parece uma receita para uma violação mais geral do princípio DRY, se você tiver vários tipos, cada um classificando suas próprias transações usando muitos dos mesmos métodos. O SOLID nem sempre se sente 100% relevante para o desenvolvedor principalmente de JavaScript (suspeito que muito disso seja explicado mal), mas o DRY é o padrão-ouro na metodologia de programação geral IMO. Sempre que me deparo com uma idéia que parece uma extensão do DRY, considero isso. SRP é um desses. Se todos permanecerem no assunto, é menos provável que você se sinta tentado a se repetir.

Se existe conceitualmente apenas uma maneira de UpdateGroupBilling(...)fazê-lo, e isso só acontece naquele lugar, provavelmente está bem onde está. Mas a questão está relacionada ao conceito, não às circunstâncias temporais, ou seja, "por enquanto, apenas o fazemos aqui".

Se esse não for o caso, uma maneira de refatorar seria usar Publicar / Assinar e notificar sempre que o pagamento for salvo, e fazer com que o faturamento se inscreva nesse evento e atualize as entradas relevantes. Isso é bom, especialmente se você precisar de vários tipos de cobrança que precisam ser atualizados. Outro método seria pensar no faturamento como um padrão de estratégia, ou seja, escolher um e usá-lo ou aceitá-lo como parâmetro. Ou, se o faturamento nem sempre acontecer, você poderá adicioná-lo como decorador, etc. etc. Mas, novamente, isso depende do seu modelo conceitual e de como você deseja pensar sobre isso, para que você tenha que descobrir isso. .

Uma coisa importante a considerar, no entanto, é o tratamento de erros. Se o faturamento falhar, o que acontece com as operações anteriores?

Acho que esse design está violando o SRP, mas é realmente fácil de corrigir e ainda fazer tudo o que for necessário.

Pense nas mensagens e no que está acontecendo neste método. Ele deve salvar o arquivo GroupBillingPayment, mas não há nada errado se GroupBillingPayment notificar as classes de que foram salvas. Especialmente se estiver implementando um padrão que exponha explicitamente esse comportamento.

Você poderia usar o padrão Observer .

Aqui está um exemplo de como isso funcionaria no seu 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);
}

Agora, tudo o que você precisa fazer é criar um ou mais Observersque serão vinculados GroupBillingPaymente, assim, ser notificado sempre que for salvo. Ele mantém suas próprias dependências, não sabe o que foi notificado e, portanto, não depende delas.

Você deseja obter o X. A menos que o X seja bastante trivial, escreva um método que faça tudo para obter o X, chame o método "reachX" e chame esse método. A responsabilidade única de "conseguirX" é fazer de tudo para alcançar X. "conseguirX" não deve fazer outras coisas.

Se X é complexo, muitas ações podem ser necessárias para alcançar X. O conjunto de ações necessárias pode mudar ao longo do tempo; portanto, quando isso acontece, você altera o método e oX, mas se tudo correr bem, você ainda pode chamá-lo.

No seu exemplo, o método "Salvar" a responsabilidade não é salvar a fatura e atualizar o faturamento do grupo (duas responsabilidades); sua responsabilidade é executar todas as ações necessárias para que a fatura seja registrada permanentemente. Uma responsabilidade. Talvez você tenha nomeado mal.

Se eu entendi o seu ponto, eu teria o pagamento para gerar um evento como "Pagamento efetuado" e a classe que gerencia o faturamento que o assina. Dessa forma, o manipulador de pagamentos não sabe nada sobre o faturamento.