Um construtor que valida seus argumentos viola o SRP?

Estou tentando aderir ao Princípio da Responsabilidade Única (SRP), tanto quanto possível, e me acostumei a um certo padrão (para o SRP de métodos), confiando fortemente nos delegados. Gostaria de saber se essa abordagem é sólida ou se há algum problema grave.

Por exemplo, para verificar a entrada de um construtor, eu poderia introduzir o seguinte método (a Streamentrada é aleatória, pode ser qualquer coisa)

private void CheckInput(Stream stream)
{
    if(stream == null)
    {
        throw new ArgumentNullException();
    }

    if(!stream.CanWrite)
    {
        throw new ArgumentException();
    }
}

Esse método (sem dúvida) faz mais de uma coisa

• Verifique as entradas
• Lance exceções diferentes

Para aderir ao SRP, mudei a lógica para

private void CheckInput(Stream stream, 
                        params (Predicate<Stream> predicate, Action action)[] inputCheckers)
{
    foreach(var inputChecker in inputCheckers)
    {
        if(inputChecker.predicate(stream))
        {
            inputChecker.action();
        }
    }
}

O que supostamente faz apenas uma coisa (faz?): Verifique a entrada. Para a verificação real das entradas e o lançamento das exceções, introduzi métodos como

bool StreamIsNull(Stream s)
{
    return s == null;
}

bool StreamIsReadonly(Stream s)
{
    return !s.CanWrite;
}

void Throw<TException>() where TException : Exception, new()
{
    throw new TException();
}

e pode ligar CheckInputcomo

CheckInput(stream,
    (this.StreamIsNull, this.Throw<ArgumentNullException>),
    (this.StreamIsReadonly, this.Throw<ArgumentException>))

Isso é melhor do que a primeira opção, ou introduzo complexidade desnecessária? Existe alguma maneira de eu ainda melhorar esse padrão, se é viável?

O SRP é talvez o princípio de software mais incompreendido.

Um aplicativo de software é construído a partir de módulos, que são construídos a partir de módulos, construídos a partir de ...

Na parte inferior, uma única função como CheckInputconterá apenas um pouquinho de lógica, mas à medida que você sobe, cada módulo sucessivo encapsula cada vez mais lógica e isso é normal .

O SRP não é sobre executar uma única ação atômica . É sobre ter uma única responsabilidade, mesmo que essa responsabilidade exija várias ações ... e, finalmente, sobre manutenção e testabilidade :

• promove o encapsulamento (evitando objetos de Deus),
• promove a separação de preocupações (evitando alterações onduladas em toda a base de código),
• ajuda a testabilidade, estreitando o escopo de responsabilidades.

O fato de CheckInputser implementado com duas verificações e gerar duas exceções diferentes é irrelevante até certo ponto.

CheckInputtem uma responsabilidade estreita: garantir que a entrada esteja em conformidade com os requisitos. Sim, existem vários requisitos, mas isso não significa que haja várias responsabilidades. Sim, você pode dividir os cheques, mas como isso ajudaria? Em algum momento, as verificações devem ser listadas de alguma forma.

Vamos comparar:

Constructor(Stream stream) {
    CheckInput(stream);
    // ...
}

versus:

Constructor(Stream stream) {
    CheckInput(stream,
        (this.StreamIsNull, this.Throw<ArgumentNullException>),
        (this.StreamIsReadonly, this.Throw<ArgumentException>));
    // ...
}

Agora, CheckInputfaz menos ... mas seu interlocutor faz mais!

Você mudou a lista de requisitos de CheckInput, onde estão encapsulados, para Constructoronde estão visíveis.

É uma boa mudança? Depende:

• Se CheckInputé chamado apenas lá: é discutível, por um lado, torna os requisitos visíveis, por outro, desorganiza o código;
• Se CheckInputfor chamado várias vezes com os mesmos requisitos , violará o DRY e você terá um problema de encapsulamento.

É importante perceber que uma única responsabilidade pode implicar muito trabalho. O "cérebro" de um carro autônomo tem uma única responsabilidade:

Dirigindo o carro para o seu destino.

É uma responsabilidade única, mas requer a coordenação de uma tonelada de sensores e atores, a tomada de muitas decisões e até possíveis requisitos conflitantes ¹ ...

... no entanto, está tudo encapsulado. Então o cliente não se importa.

¹ segurança dos passageiros, segurança dos outros, respeito aos regulamentos, ...

Citando o tio Bob sobre o SRP ( https://8thlight.com/blog/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html ):

O Princípio de Responsabilidade Única (SRP) afirma que cada módulo de software deve ter um e apenas um motivo para mudar.

... Este princípio é sobre pessoas.

... Quando você escreve um módulo de software, deseja garantir que, quando as alterações forem solicitadas, essas alterações possam se originar apenas de uma única pessoa, ou melhor, de um único grupo de pessoas fortemente associado que representa uma única função de negócios definida de maneira restrita.

... É por isso que não colocamos SQL em JSPs. É por isso que não geramos HTML nos módulos que calculam os resultados. Esse é o motivo pelo qual as regras de negócios não devem conhecer o esquema do banco de dados. Esta é a razão pela qual separamos as preocupações.

Ele explica que os módulos de software devem abordar preocupações específicas das partes interessadas. Portanto, respondendo sua pergunta:

Isso é melhor do que a primeira opção, ou introduzo complexidade desnecessária? Existe alguma maneira de eu ainda melhorar esse padrão, se é viável?

Na IMO, você está analisando apenas um método, quando deve procurar um nível superior (neste caso, nível de classe). Talvez devêssemos dar uma olhada no que sua classe está fazendo atualmente (e isso exige mais explicações sobre o seu cenário). Por enquanto, sua turma ainda está fazendo a mesma coisa. Por exemplo, se amanhã houver alguma solicitação de alteração sobre alguma validação (por exemplo: "agora o fluxo pode ser nulo"), você ainda precisará ir para essa classe e alterar as coisas nela.

Não, essa alteração não é informada pelo SRP.

Pergunte a si mesmo por que não há verificação no seu verificador para "o objeto passado é um fluxo" . A resposta é óbvia: o idioma impede que o chamador compile um programa que passa em um não fluxo.

O sistema de tipos de C # é insuficiente para atender às suas necessidades; suas verificações estão implementando a imposição de invariantes que não podem ser expressos no sistema de tipos hoje . Se houvesse uma maneira de dizer que o método usa um fluxo gravável não anulável, você teria escrito isso, mas não existe, então fez a próxima melhor coisa: aplicou a restrição de tipo no tempo de execução. Espero que você também o tenha documentado, para que os desenvolvedores que usam seu método não precisem violá-lo, falhar em seus casos de teste e corrigir o problema.

Colocar tipos em um método não é uma violação do Princípio da Responsabilidade Única; nem o método está impondo suas pré-condições ou afirmando suas pós-condições.

Nem todas as responsabilidades são criadas iguais.

Aqui estão duas gavetas. Ambos têm uma responsabilidade. Cada um deles tem nomes que informam o que pertence a eles. Uma é a gaveta de talheres. O outro é a gaveta de lixo eletrônico.

Então qual a diferença? A gaveta de talheres deixa claro o que não pertence a ela. A gaveta de lixo eletrônico, no entanto, aceita qualquer coisa que caiba. Retirar as colheres da gaveta dos talheres parece muito errado. No entanto, tenho dificuldade em pensar em qualquer coisa que seria perdida se removida da gaveta do lixo. A verdade é que você pode afirmar que algo tem uma única responsabilidade, mas qual você acha que tem a responsabilidade única mais focada?

Um objeto com uma única responsabilidade não significa que apenas uma coisa pode acontecer aqui. Responsabilidades podem aninhar. Mas essas responsabilidades de nidificação devem fazer sentido, não devem surpreendê-lo quando você as encontrar aqui e você deve sentir falta delas se elas se foram.

Então, quando você oferece

CheckInput(Stream stream);

Não me preocupo com o fato de verificar as entradas e lançar exceções. Eu ficaria preocupado se fosse verificar a entrada e salvar entrada também. Que surpresa desagradável. Um que eu não sentiria falta se tivesse sumido.

Quando você se amarra e escreve um código estranho para estar em conformidade com um Princípio Importante do Software, geralmente você não entendeu o princípio (embora às vezes o princípio esteja errado). Como a excelente resposta de Matthieu aponta, todo o significado de SRP depende da definição de "responsabilidade".

Programadores experientes veem esses princípios e os relacionam com memórias de código que estragamos; programadores menos experientes os veem e podem não ter nada com o que se relacionar. É uma abstração flutuando no espaço, todo sorriso e nenhum gato. Então eles adivinham, e geralmente vai mal. Antes de você desenvolver o senso de programação, a diferença entre código estranho e complicado demais e código normal não é de todo óbvia.

Este não é um mandamento religioso que você deve obedecer, independentemente das consequências pessoais. É mais uma regra prática destinada a formalizar um elemento da programação do senso de cavalo e ajudar a manter seu código o mais simples e claro possível. Se está tendo o efeito oposto, você está certo em procurar alguma entrada externa.

Na programação, você não pode errar muito mais do que tentar deduzir o significado de um identificador dos primeiros princípios apenas olhando para ele, e isso vale para os identificadores que escrevem sobre programação tanto quanto para os identificadores no código real.

Função CheckInput

Primeiro, deixe-me dizer o óbvio, CheckInput está fazendo uma coisa, mesmo que esteja verificando vários aspectos. Por fim, verifica a entrada . Alguém poderia argumentar que não é uma coisa se você estiver lidando com métodos chamados DoSomething, mas acho que é seguro assumir que a verificação de entrada não é muito vaga.

Adicionar esse padrão para predicados pode ser útil se você não quiser que a lógica para verificar a entrada seja colocada em sua classe, mas esse padrão parece bastante detalhado para o que você está tentando alcançar. Pode ser muito mais direto simplesmente passar uma interface IStreamValidatorcom método único, isValid(Stream)se é isso que você deseja obter. Qualquer implementação de classe IStreamValidatorpode usar predicados como StreamIsNullou StreamIsReadonlyse assim o desejarem, mas voltando ao ponto central, é uma mudança ridícula fazer no interesse de manter o princípio de responsabilidade única.

Verificação de sanidade

É minha ideia que todos nós possamos fazer uma "verificação de sanidade" para garantir que você esteja lidando com um fluxo que não seja nulo e que possa ser gravado, e essa verificação básica não está de alguma forma fazendo de sua classe um validador de fluxos. Lembre-se, é melhor deixar cheques mais sofisticados fora da sua turma, mas é aí que a linha é traçada. Depois de começar a mudar o estado do seu fluxo lendo-o ou dedicando recursos para a validação, você começa a executar uma validação formal do seu fluxo e é isso que deve ser inserido em sua própria classe.

Conclusão

Penso que, se você estiver aplicando um padrão para organizar melhor um aspecto de sua classe, ele merece estar em sua própria classe. Como um padrão não se encaixa, você também deve questionar se ele realmente pertence ou não a sua própria classe em primeiro lugar. Penso que, a menos que você acredite que a validação do fluxo provavelmente será alterada no futuro, e especialmente se você acredita que essa validação pode até ser de natureza dinâmica, o padrão que você descreveu é uma boa idéia, mesmo que possa ser inicialmente trivial. Caso contrário, não há necessidade de arbitrariamente tornar seu programa mais complexo. Vamos chamar uma pá de pá. A validação é uma coisa, mas verificar a entrada nula não é validação e, portanto, acho que você pode estar seguro em mantê-la em sua classe sem violar o princípio da responsabilidade única.

O princípio enfaticamente não afirma que um pedaço de código deve "fazer apenas uma única coisa".

"Responsabilidade" no SRP deve ser entendida no nível dos requisitos. A responsabilidade do código é atender aos requisitos de negócios. O SRP é violado se um objeto atender a mais de um requisito de negócios independente . Por independente, significa que um requisito pode mudar enquanto o outro requisito permanece no local.

É concebível que um novo requisito comercial seja introduzido, o que significa que esse objeto em particular não deve ser legível, enquanto outro requisito comercial ainda exige que o objeto seja legível? Não, porque os requisitos de negócios não especificam detalhes da implementação nesse nível.

Um exemplo real de uma violação de SRP seria um código como este:

var message = "Your package will arrive before " + DateTime.Now.AddDays(14);

Esse código é muito simples, mas ainda é concebível que o texto seja alterado independentemente da data de entrega prevista, pois estes são decididos por diferentes partes do negócio.

Gosto do ponto da resposta de @ EricLippert :

Pergunte a si mesmo por que não há verificação no seu verificador, pois o objeto passado é um fluxo . A resposta é óbvia: o idioma impede que o chamador compile um programa que passa em um não fluxo.

O sistema de tipos de C # é insuficiente para atender às suas necessidades; suas verificações estão implementando a imposição de invariantes que não podem ser expressos no sistema de tipos hoje . Se houvesse uma maneira de dizer que o método usa um fluxo gravável não anulável, você teria escrito isso, mas não existe, então fez a próxima melhor coisa: aplicou a restrição de tipo no tempo de execução. Espero que você também o tenha documentado, para que os desenvolvedores que usam seu método não precisem violá-lo, falhar em seus casos de teste e corrigir o problema.

EricLippert está certo de que este é um problema para o sistema de tipos. E como você deseja usar o princípio de responsabilidade única (SRP), basicamente precisa do sistema de tipos para ser responsável por esse trabalho.

Na verdade, é possível fazer isso em C #. Podemos capturar literais nullno tempo de compilação e capturar literais nullnão no tempo de execução. Isso não é tão bom quanto uma verificação completa em tempo de compilação, mas é uma melhoria estrita do que nunca é capturada em tempo de compilação.

Então, você sabe como o C # tem Nullable<T>? Vamos reverter isso e fazer um NonNullable<T>:

public struct NonNullable<T> where T : class
{
    public T Value { get; private set; }
    public NonNullable(T value)
    {
        if (value == null) { throw new NullArgumentException(); }
        this.Value = value;
    }
    //  Ease-of-use:
    public static implicit operator T(NonNullable<T> value) { return value.Value; }
    public static implicit operator NonNullable<T>(T value) { return new NonNullable<T>(value); }

    //  Hack-ish overloads that prevent null-literals from being implicitly converted into NonNullable<T>'s.
    public static implicit operator NonNullable<T>(Tuple<T> value) { return new NonNullable<T>(value.Item1); }
    public static implicit operator NonNullable<T>(Tuple<T, T> value) { return new NonNullable<T>(value.Item1); }
}

Agora, em vez de escrever

public void Foo(Stream stream)
{
  if (stream == null) { throw new NullArgumentException(); }

  // ...method code...
}

, apenas escreva:

public void Foo(NonNullable<Stream> stream)
{
  // ...method code...
}

Então, existem três casos de uso:

1. O usuário chama Foo()com um valor não nulo Stream:

   Stream stream = new Stream();
   Foo(stream);

   Este é o caso de uso desejado e funciona com ou sem NonNullable<>.

2. O usuário chama Foo()com um nulo Stream:

   Stream stream = null;
   Foo(stream);

   Este é um erro de chamada. Aqui, NonNullable<>ajuda a informar o usuário que ele não deveria estar fazendo isso, mas na verdade não o impede. De qualquer forma, isso resulta em tempo de execução NullArgumentException.

3. O usuário liga Foo()com null:

   Foo(null);

   nullnão será convertido implicitamente em um NonNullable<>, para que o usuário receba um erro no IDE antes do tempo de execução. Isso está delegando a verificação nula no sistema de tipos, exatamente como o SRP recomendaria.

Você pode estender esse método para afirmar outras coisas sobre seus argumentos também. Por exemplo, como você deseja um fluxo gravável, é possível definir um struct WriteableStream<T> where T:Streamque verifique ambos nulle stream.CanWriteo construtor. Ainda seria uma verificação do tipo em tempo de execução, mas:

1. Decora o tipo com o WriteableStreamqualificador, sinalizando a necessidade de chamadas.

2. Ele faz a verificação em um único local no código, para que você não precise repetir a verificação throw InvalidArgumentExceptionsempre.

3. Ele está em melhor conformidade com o SRP, empurrando as tarefas de verificação de tipo para o sistema de tipos (conforme estendido pelos decoradores genéricos).

Sua abordagem é atualmente processual. Você está desmembrando o Streamobjeto e validando-o de fora. Não faça isso - ele quebra o encapsulamento. Seja Streamresponsável por sua própria validação. Não podemos procurar aplicar o SRP até termos algumas classes para aplicá-lo.

Aqui está um Streamque executa uma ação apenas se passar na validação:

class Stream
{
    public void someAction()
    {
        if(!stream.canWrite)
        {
            throw new ArgumentException();
        }

        System.out.println("My action");
    }
}

Mas agora estamos violando o SRP! "Uma turma deve ter apenas um motivo para mudar." Temos uma mistura de 1) validação e 2) lógica real. Temos dois motivos pelos quais ele pode precisar mudar.

Podemos resolver isso com validadores de decoradores . Primeiro, precisamos converter nossa Streamem uma interface e implementá-la como uma classe concreta.

interface Stream
{
    void someAction();
}

class DefaultStream implements Stream
{
    @Override
    public void someAction()
    {
        System.out.println("My action");
    }
}

Agora podemos escrever um decorador que envolve a Stream, executa a validação e adia o que foi dado Streampara a lógica real da ação.

class WritableStream implements Stream
{
    private final Stream stream;

    public WritableStream(final Stream stream)
    {
        this.stream = stream;
    }

    @Override
    public void someAction()
    {
        if(!stream.canWrite)
        {
            throw new ArgumentException();
        }
        stream.someAction();
    }
}

Agora podemos compor esses da maneira que quisermos:

final Stream myStream = new WritableStream(
    new DefaultStream()
);

Deseja validação adicional? Adicione outro decorador.

O trabalho de uma classe é fornecer um serviço que atenda a um contrato . Uma classe sempre tem um contrato: um conjunto de requisitos para usá-lo e promete fazer seu estado e resultados, desde que os requisitos sejam atendidos. Este contrato pode ser explícito, através de documentação e / ou asserções, ou implícito, mas sempre existe.

Parte do contrato da sua classe é que o chamador fornece ao construtor alguns argumentos que não devem ser nulos. A implementação do contrato é de responsabilidade da classe; portanto, verificar se o chamador cumpriu sua parte do contrato pode ser facilmente considerado como estando dentro do escopo de responsabilidade da classe.

A idéia de que uma classe implementa um contrato deve-se a Bertrand Meyer , o designer da linguagem de programação Eiffel e da idéia de design por contrato . A linguagem Eiffel torna a especificação e a verificação do contrato parte da linguagem.

Como foi apontado em outras respostas, o SRP geralmente é mal compreendido. Não se trata de ter código atômico que apenas faz uma função. Trata-se de garantir que seus objetos e métodos façam apenas uma coisa e que a única coisa seja feita em um só lugar.

Vamos ver um exemplo ruim em pseudo-código.

class Math
    private int a;
    private int b;
    def constructor(int x, int y) 
        if(x != null)
          a = x
        else if(x < 0)
          a = abs(x)
        else if (x == -1)
          throw "Some Silly Error"
        else
          a = 0
        end
        if(y != null)
           b = y
        else if(y < 0)
           b = abs(y)
        else if(y == -1)
           throw "Some Silly Error"
        else
         b = 0
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

No nosso exemplo absurdo, a "responsabilidade" do construtor Math # é tornar o objeto matemático utilizável. Isso é feito limpando primeiro a entrada e depois certificando-se de que os valores não sejam -1.

Isso é SRP válido porque o construtor está fazendo apenas uma coisa. Está preparando o objeto Math. No entanto, não é muito sustentável. Viola SECO.

Então, vamos dar outro passo nisso

class Math
    private int a;
    private int b;
    def constructor(int x, int y)
        cleanX(x)
        cleanY(y)
    end
    def cleanX(int x)
        if(x != null)
          a = x
        else if(x < 0)
          a = abs(x)
        else if (x == -1)
          throw "Some Silly Error"
        else
          a = 0
        end
   end
   def cleanY(int y)
        if(y != null)
           b = y
        else if(y < 0)
           b = abs(y)
        else if(y == -1)
           throw "Some Silly Error"
        else
         b = 0
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

Neste passo, melhoramos um pouco o DRY, mas ainda temos um caminho a percorrer com o DRY. O SRP, por outro lado, parece um pouco errado. Agora, temos duas funções com o mesmo trabalho. CleanX e cleanY limpam a entrada.

Vamos tentar de novo

class Math
    private int a;
    private int b;
    def constructor(int x, int y)
        a = clean(x)
        b = clean(y)
    end
    def clean(int i)
        if(i != null)
          return i
        else if(i < 0)
          return abs(i)
        else if (i == -1)
          throw "Some Silly Error"
        else
          return 0
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

Agora, finalmente, foram melhores sobre DRY, e SRP parece estar de acordo. Temos apenas um lugar que realiza o trabalho de "higienizar".

O código é, em teoria, mais sustentável e melhor ainda, quando vamos corrigir o bug e reforçar o código, precisamos apenas fazê-lo em um só lugar.

class Math
    private int a;
    private int b;
    def constructor(int x, int y)
        a = clean(x)
        b = clean(y)
    end
    def clean(int i)
        if(i == null)
          return 0
        else if (i == -1)
          throw "Some Silly Error"
        else
          return abs(i)
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

Na maioria dos casos do mundo real, os objetos seriam mais complexos e o SRP seria aplicado em vários objetos. Por exemplo, a idade pode pertencer a Pai, Mãe, Filho, Filha; portanto, em vez de ter 4 classes que determinam a idade a partir da data de nascimento, você tem uma classe Person que faz isso e as 4 classes herdam disso. Mas espero que este exemplo ajude a explicar. O SRP não é sobre ações atômicas, mas sobre um "trabalho" sendo realizado.

Falando em SRP, o tio Bob não gosta de cheques nulos espalhados por toda parte. Em geral, você, como equipe, deve evitar o uso de parâmetros nulos para construtores sempre que possível. Quando você publica seu código fora da sua equipe, as coisas podem mudar.

Aplicar a não anulabilidade dos parâmetros do construtor sem primeiro garantir a coesão da classe em questão resulta em inchaço no código de chamada, especialmente nos testes.

Se você realmente deseja fazer cumprir tais contratos, considere usar Debug.Assertalgo semelhante para reduzir a desordem:

public AClassThatDefinitelyNeedsAWritableStream(Stream stream)
{
   Assert.That(stream.CanWrite, "Put crucial information here, and not inane bloat.");

   // Go on normal operation.
}