Um construtor que valide seus argumentos viola o SRP?

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

Uma aplicação de software é construída a partir de módulos, que são construídos a partir de módulos, que são construídos a partir de ...

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

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

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

O fato de que CheckInput é implementado com duas verificações e gera duas exceções diferentes é irrelevante até certo ponto.

CheckInput tem uma responsabilidade limitada: garantir que a entrada atenda aos requisitos. Sim, existem vários requisitos, mas isso não significa que existam várias responsabilidades. Sim, você poderia 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, CheckInput faz menos ... mas o chamador faz mais!

Você alterou a lista de requisitos de CheckInput , onde eles estão encapsulados, para Constructor onde eles estão visíveis.

É uma boa mudança? Depende:

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

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

Driving the car to its destination.

É uma responsabilidade única, mas requer coordenar uma tonelada de sensores e atores, tomar muitas decisões e até mesmo ter requisitos conflitantes ¹ ...

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

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

Citando o tio Bob sobre o SRP ( link ):

The Single Responsibility Principle (SRP) states that each software module should have one and only one reason to change.

... This principle is about people.

... When you write a software module, you want to make sure that when changes are requested, those changes can only originate from a single person, or rather, a single tightly coupled group of people representing a single narrowly defined business function.

... This is the reason we do not put SQL in JSPs. This is the reason we do not generate HTML in the modules that compute results. This is the reason that business rules should not know the database schema. This is the reason we separate concerns.

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

Is this any better than the first option at all, or do I introduce unneccesary complexity? Is there any way I can still improve this pattern, if it's viable at all?

IMO, você está apenas olhando para um método, quando deveria olhar para um nível mais alto (nível de classe, neste caso). Talvez devêssemos dar uma olhada no que sua turma está fazendo atualmente (e isso requer mais explicações sobre o seu cenário). Por enquanto, sua turma ainda está fazendo a mesma coisa. Por exemplo, se amanhã houver alguma requisição de mudança sobre alguma validação (por exemplo: "agora o fluxo pode ser nulo"), então você ainda precisa ir para esta classe e mudar coisas dentro dela.

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

Pergunte-se por que não há verificação em seu verificador para "o objeto passado é um fluxo" . A resposta é óbvia: a linguagem evita 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 expressas no sistema de tipos hoje . Se houvesse uma maneira de dizer que o método recebe um fluxo gravável não anulável, você teria escrito isso, mas não há, então você fez a próxima melhor coisa: você aplicou a restrição de tipo em tempo de execução. Espero que você também tenha documentado, para que os desenvolvedores que usam seu método não tenham que 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 de Responsabilidade Única; o método também não reforça suas pré-condições ou afirma 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 tem nomes que lhe permitem saber o que pertence a eles. Uma é a gaveta de talheres. O outro é a gaveta de lixo.

Então, qual é a diferença? A gaveta de talheres deixa claro o que não pertence a ela. A gaveta de lixo, no entanto, aceita qualquer coisa que caiba. Tirar as colheres da gaveta de talheres parece muito errado. No entanto, tenho dificuldade em pensar em algo que seria perdido se fosse removido da gaveta de lixo. A verdade é que você pode afirmar que qualquer coisa 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, elas não devem surpreendê-lo quando você as encontrar aqui e você deve sentir falta delas se elas forem embora.

Então, quando você oferece

CheckInput(Stream stream);

Eu não me preocupo com a possibilidade de verificar a entrada e lançar exceções. Eu ficaria preocupado se fosse ao mesmo tempo verificar a entrada e salvar a entrada também. Isso é uma surpresa desagradável. Um que eu não sentiria falta se tivesse sumido.

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

Programadores experientes vêem esses princípios e os relacionam com memórias de código que estragamos; programadores menos experientes os vêem e podem não ter nada para relacioná-los. É uma abstração flutuando no espaço, todo sorriso e nenhum gato. Então eles adivinham, e geralmente vai mal. Antes de você ter desenvolvido o senso de cavalo de programação, a diferença entre o estranho código supercomplicado e o código normal não é nada óbvio.

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

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

Função CheckInput

Primeiro, deixe-me colocar o óbvio lá fora, CheckInput está fazendo uma coisa, mesmo que esteja verificando vários aspectos. Em última análise, verifica a entrada . Pode-se argumentar que não é uma coisa se você está lidando com métodos chamados DoSomething , mas eu acho que é seguro assumir que a verificação de entrada não é muito vaga.

Adicionar este 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 IStreamValidator com o método único isValid(Stream) se é isso que você deseja obter. Qualquer classe implementando IStreamValidator pode usar predicados como StreamIsNull ou StreamIsReadonly , se desejar, mas voltando ao ponto central, é uma mudança bastante ridícula fazer no interesse de manter o princípio da responsabilidade única.

Verificação de sanidade

É minha ideia que todos nós recebamos uma "verificação de sanidade" para garantir que você esteja, pelo menos, lidando com um Fluxo não nulo e gravável, e essa verificação básica não está de alguma forma tornando sua classe < em> validador dos fluxos. Lembre-se, seria melhor deixar verificações mais sofisticadas fora de sua classe, mas é aí que a linha é desenhada. Depois que você precisar começar a alterar o estado de seu fluxo lendo ou dedicando recursos à validação, você começou a executar uma validação formal de seu fluxo e este é o que deve ser puxado para sua própria classe.

Conclusão

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

O princípio enfaticamente não declara que um trecho de código deveria "fazer apenas uma única coisa".

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

É concebível que um novo requisito de negócios seja introduzido, o que significa que esse objeto específico não deve verificar se está legível, enquanto outro requisito de negócios ainda requer que o objeto verifique se está legível? Não, porque os requisitos de negócios não especificam detalhes de 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);

Este código é muito simples, mas ainda assim é concebível que o texto mude de forma independente da data de entrega esperada, uma vez que estes são decididos por diferentes partes do negócio.

Eu gosto do ponto da resposta @ EricLippert :

Ask yourself why there is no check in your checker for the object passed in is a stream. The answer is obvious: the language prevents the caller from compiling a program that passes in a non-stream.

The type system of C# is insufficient to meet your needs; your checks are implementing enforcement of invariants that cannot be expressed in the type system today. If there was a way to say that the method takes a non-nullable writeable stream, you'd have written that, but there isn't, so you did the next best thing: you enforced the type restriction at runtime. Hopefully you also documented it, so that developers who use your method do not have to violate it, fail their test cases, and then fix the problem.

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

É realmente possível fazer isso em C #. Nós podemos pegar literal null em tempo de compilação, então pegar não-literal null em tempo de execução. Isso não é tão bom quanto uma checagem completa em tempo de compilação, mas é uma melhoria estrita sobre nunca pegar em tempo de compilação.

Então, você sabe como o C # tem Nullable<T> ? Vamos inverter 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...
}

, escreva:

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

Depois, há três casos de uso:

1. Chamadas de usuário Foo() com um Stream não-nulo:

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

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

2. Chamadas de usuário Foo() com um% nuloStream:

   Stream stream = null;
   Foo(stream);
   

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

3. Chamadas do usuário Foo() com null :

   Foo(null);
   

   null não é convertido implicitamente em NonNullable<> , portanto, o usuário recebe um erro no IDE, antes em tempo de execução. Isso é delegar a verificação nula ao sistema de tipos, exatamente como o SRP aconselharia.

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:Stream que verifica os null e stream.CanWrite no construtor. Isso ainda seria uma verificação de tipo de tempo de execução, mas:

1. Decora o tipo com o qualificador WriteableStream , sinalizando a necessidade dos chamadores.

2. Ele faz a verificação em um único local no código, portanto, você não precisa repetir a verificação e throw InvalidArgumentException de cada vez.

3. É melhor estar em conformidade com o SRP, enviando as tarefas de verificação de tipos para o sistema de tipos (conforme estendido pelos decoradores genéricos).

Sua abordagem é atualmente processual. Você está separando o objeto Stream e validando-o do lado de fora. Não faça isso - quebra o encapsulamento. Deixe o Stream ser responsável por sua própria validação. Não podemos tentar aplicar o SRP até que tenhamos algumas classes para aplicá-lo.

Aqui está um Stream que 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." Nós temos um mix de 1) validação e 2) lógica real. Temos dois motivos para mudar.

Podemos resolver isso com validando decoradores . Primeiro, precisamos converter nosso Stream em uma interface e implementá-lo 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 contenha um Stream , realize a validação e adie para o dado Stream para 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 tudo de que gostamos:

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

Quer validação adicional? Adicione outro decorador.

O trabalho de uma turma é fornecer um serviço que atenda a um contrato . Uma classe sempre tem um contrato: um conjunto de requisitos para usá-lo e promessas que ele faz sobre seu estado e saídas, desde que os requisitos sejam atendidos. Este contrato pode ser explícito, por meio de documentação e / ou asserções, ou implícito, mas sempre existe.

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

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

Como foi apontado em outras respostas, o SRP é freqüentemente mal entendido. Não se trata de ter código atômico que faz apenas 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 no pseudocó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

Em nosso exemplo um tanto absurdo, a "responsabilidade" do construtor Math # é tornar o objeto matemático utilizável. Ele faz isso primeiro desinfetando a entrada e, em seguida, certificando-se de que os valores não sejam -1.

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

Então vamos dar outro passo para isso

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 passe ficamos um pouco melhor sobre DRY, mas ainda temos algumas maneiras de usar o DRY. SRP por outro lado parece um pouco fora. Agora temos duas funções com o mesmo trabalho. Ambos cleanX e cleanY sanitizam a entrada.

Vamos dar outra chance

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, foi melhor sobre DRY e o SRP parece estar de acordo. Temos apenas um lugar que faz o trabalho "higienizar".

O código é, em teoria, mais sustentável e, melhor ainda, quando vamos corrigir o bug e apertar o código, só precisamos 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, então ao invés de ter 4 classes que calculam 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 feito.

Falando em SRP, o Tio Bob não gosta de verificações nulas espalhadas por toda parte. Em geral, você, como equipe, deve evitar usar parâmetros nulos para construtores sempre que possível. Quando você publica seu código fora de sua equipe, as coisas podem mudar.

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

Se você realmente quiser impor esses contratos, considere usar Debug.Assert ou algo 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.
}