Un costruttore che convalida i suoi argomenti viola SRP?

SRP è forse il principio del software più frainteso.

Un'applicazione software è costruita da moduli, che sono costruiti da moduli, costruiti da ...

In basso, una singola funzione come CheckInput conterrà solo un piccolo bit di logica, ma man mano che si procede verso l'alto, ogni modulo successivo incapsula sempre più logici e questo è normale .

SRP non sta facendo una singola azione atomica . Si tratta di una singola responsabilità, anche se tale responsabilità richiede più azioni ... e in definitiva si tratta di manutenzione e testabilità :

• promuove l'incapsulamento (evitando oggetti di Dio),
• promuove la separazione delle preoccupazioni (evitando modifiche increspature attraverso l'intera base di codice),
• aiuta la testabilità riducendo la portata delle responsabilità.

Il fatto che CheckInput sia implementato con due verifiche e innalzi due diverse eccezioni è irrilevante in una certa misura.

CheckInput ha una responsabilità limitata: garantire che l'input sia conforme ai requisiti. Sì, ci sono più requisiti, ma questo non significa che ci sono più responsabilità. Sì, potresti dividere gli assegni, ma come ti sarebbe di aiuto? Ad un certo punto i controlli devono essere elencati in qualche modo.

Confrontiamo:

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

vs

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

Ora, CheckInput fa meno ... ma il suo chiamante fa di più!

Hai spostato l'elenco dei requisiti da CheckInput , dove sono incapsulati, a Constructor dove sono visibili.

È un bel cambiamento? Dipende:

• Se CheckInput viene solo chiamato lì: è discutibile, da un lato rende visibili i requisiti, dall'altro ingombra il codice;
• Se CheckInput viene chiamato più volte con gli stessi requisiti , allora viola DRY e hai un problema di incapsulamento.

È importante rendersi conto che una singola responsabilità può implicare un lotto di lavoro. Il "cervello" di un'auto a guida autonoma ha una sola responsabilità:

Driving the car to its destination.

È una singola responsabilità, ma richiede il coordinamento di una tonnellata di sensori e attori, prendendo molta decisione, e magari ha anche requisiti conflittuali ¹ ...

... tuttavia, è tutto incapsulato. Quindi al cliente non interessa.

¹ sicurezza dei passeggeri, sicurezza degli altri, rispetto dei regolamenti, ...

Citando Zio Bob riguardo al 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.

Spiega che i moduli software devono affrontare le preoccupazioni specifiche degli stakeholder. Pertanto, rispondendo alla tua domanda:

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, stai solo guardando un metodo, quando dovresti guardare un livello più alto (livello di classe in questo caso). Forse dovremmo dare un'occhiata a ciò che sta facendo la tua classe (e questo richiede più spiegazioni sul tuo scenario). Per ora, la tua classe sta ancora facendo la stessa cosa. Ad esempio, se domani ci sono alcune richieste di modifica su alcune convalide (ad es .: "ora lo stream può essere nullo"), allora devi ancora andare in questa classe e cambiare le cose al suo interno.

No, questo cambiamento non è informato dall'SRP.

Chiediti perché non c'è alcun controllo nel tuo correttore per "l'oggetto passato è uno stream" . La risposta è ovvia: la lingua impedisce al chiamante di compilare un programma che passi in un non-stream.

Il sistema di tipi di C # non è sufficiente per soddisfare le tue esigenze; i tuoi controlli sono che implementano l'applicazione di invarianti che non possono essere espressi nel sistema dei tipi oggi . Se ci fosse un modo per dire che il metodo prende un flusso scrivibile non annullabile, lo avresti scritto, ma non c'è, quindi hai fatto la cosa migliore: hai applicato la restrizione del tipo in fase di esecuzione. Spero che l'abbia anche documentato, in modo che gli sviluppatori che usano il tuo metodo non debbano violarlo, fallire i casi di test e quindi risolvere il problema.

Mettere i tipi su un metodo non è una violazione del Principio di Responsabilità Unica; nessuno dei due è il metodo che impone le sue precondizioni o afferma le sue postcondizioni.

Non tutte le responsabilità sono uguali.

Qui ci sono due cassetti. Entrambi hanno una responsabilità. Ognuno di loro ha nomi che ti permettono di sapere cosa ci appartiene. Uno è il cassetto dell'argenteria. L'altro è il cassetto della spazzatura.

Quindi qual è la differenza? Il cassetto argenteria rende chiaro ciò che non gli appartiene. Il cassetto della spazzatura accetta comunque tutto ciò che si adatta. Prendere i cucchiai dal cassetto degli argentati sembra molto sbagliato. Eppure mi viene difficile pensare a tutto ciò che sarebbe mancato se rimosso dal cassetto della spazzatura. La verità è che puoi affermare che ogni cosa ha una singola responsabilità, ma quale pensi abbia la responsabilità singola più focalizzata?

Un oggetto che ha una singola responsabilità non significa che solo una cosa può accadere qui. Le responsabilità possono nidificare. Ma quelle responsabilità di nidificazione dovrebbero avere senso, non dovrebbero sorprenderti quando li trovi qui e ti mancheranno se non ci fossero.

Quindi quando offri

CheckInput(Stream stream);

Non mi rendo conto che sia il controllo dell'input sia il lancio di eccezioni. Sarei preoccupato se fosse sia il controllo dell'input sia il salvataggio dell'input. Questa è una brutta sorpresa. Uno che non vorrei perdere se fosse andato.

Quando ti leghi in nodi e scrivi un codice strano per conformarti a un importante principio del software, di solito hai frainteso il principio (anche se a volte il principio è sbagliato). Come sottolinea l'eccellente risposta di Matthieu, l'intero significato di SRP dipende dalla definizione di "responsabilità".

I programmatori esperti vedono questi principi e li mettono in relazione con i ricordi del codice che abbiamo rovinato; i programmatori meno esperti li vedono e potrebbero non avere nulla a cui collegarli. È un'astrazione che fluttua nello spazio, tutto sorriso e niente gatto. Quindi loro indovinano, e di solito va male. Prima di aver sviluppato la programmazione del senso del cavallo, la differenza tra il codice complicato e il codice normale non è affatto ovvia.

Questo non è un comandamento religioso che devi obbedire a prescindere dalle conseguenze personali. È più che altro una regola empirica per formalizzare un elemento di programmazione del senso del cavallo e aiutarti a mantenere il codice il più semplice e chiaro possibile. Se sta avendo l'effetto opposto, hai ragione a cercare qualche input esterno.

Nella programmazione, non si può andare molto più male di cercare di dedurre il significato di un identificatore dai primi principi semplicemente fissandolo, e questo vale per gli identificatori nello scrivere sulla programmazione tanto quanto come identificatori nel codice reale.

Ruolo CheckInput

In primo luogo, lascia che metta l'ovvio là fuori, CheckInput sta facendo una cosa, anche se sta controllando vari aspetti. Alla fine controlla l'input . Si potrebbe obiettare che non è una cosa se si ha a che fare con metodi chiamati DoSomething , ma penso che sia sicuro assumere che l'input di controllo non sia troppo vago.

L'aggiunta di questo modello per i predicati potrebbe essere utile se non si desidera che la logica per il controllo dell'input sia inserita nella classe, ma questo schema sembra piuttosto prolisso per ciò che si sta tentando di ottenere. Potrebbe essere molto più diretto passare semplicemente un'interfaccia IStreamValidator con il metodo singolo isValid(Stream) se è ciò che si desidera ottenere. Qualsiasi classe che implementa IStreamValidator può utilizzare predicati come StreamIsNull o StreamIsReadonly se lo desiderano, ma tornando al punto centrale, è un cambiamento piuttosto ridicolo da fare nell'interesse del mantenimento del principio di responsabilità singola.

Controllo Sanity

È mia opinione che ci sia concesso un "controllo di integrità" per assicurarci che tu abbia almeno a che fare con uno Stream che non è nullo e scrivibile, e questo controllo di base non sta in qualche modo rendendo la tua classe < em> validatore dei flussi. Intendiamoci, sarebbe meglio lasciare controlli più sofisticati al di fuori della vostra classe, ma è qui che viene tracciata la linea. Una volta che hai bisogno di iniziare a cambiare lo stato del tuo stream leggendo da esso o dedicando risorse alla convalida, hai iniziato a eseguire una convalida formale del tuo stream e questo è ciò che dovrebbe essere tirato nella sua classe.

Conclusione

I miei pensieri sono che se stai applicando un modello per organizzare meglio un aspetto della tua classe, merita di essere nella sua classe. Dal momento che un pattern non si adatta, dovresti anche chiedertelo se, in primo luogo, appartenga effettivamente o meno alla sua classe. I miei pensieri sono che, a meno che tu creda che la convalida del flusso probabilmente cambierà in futuro, e specialmente se ritieni che questa convalida possa anche essere di natura dinamica, allora lo schema che hai descritto è una buona idea, anche se potrebbe essere inizialmente banale In caso contrario, non è necessario rendere arbitrariamente più complesso il programma. Consente di chiamare a picche una vanga. La validazione è una cosa, ma il controllo dell'input nullo non è una convalida, e quindi penso che tu possa essere sicuro di mantenerlo nella tua classe senza violare il principio della responsabilità unica.

Il principio enfaticamente non afferma che un pezzo di codice dovrebbe "fare solo una cosa".

La "responsabilità" nell'SRP dovrebbe essere compresa a livello dei requisiti. La responsabilità del codice è soddisfare i requisiti aziendali. SRP viene violato se un oggetto soddisfa più di requisiti aziendali indipendenti . Indipendentemente significa che un requisito potrebbe cambiare mentre l'altro requisito rimane in vigore.

È ipotizzabile che venga introdotto un nuovo requisito aziendale che significa che questo particolare oggetto non dovrebbe verificare la leggibilità, mentre un altro requisito aziendale richiede ancora che l'oggetto verifichi la leggibilità? No, perché i requisiti aziendali non specificano dettagli di implementazione a quel livello.

Un esempio reale di violazione SRP sarebbe un codice come questo:

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

Questo codice è molto semplice, ma è comunque possibile che il testo cambierà indipendentemente dalla data di consegna prevista, poiché questi sono decisi da diverse parti del business.

Mi piace il punto della risposta di @ 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 ha ragione che questo è un problema per il sistema di tipi. E dal momento che si desidera utilizzare il principio di responsabilità singola (SRP), in pratica è necessario che il sistema di tipi sia responsabile di questo lavoro.

In realtà è possibile farlo in C #. Possiamo prendere letteralmente null al momento della compilazione, quindi prendere% non_alfabeti% in fase di esecuzione. Non è buono come un controllo completo in fase di compilazione, ma è un netto miglioramento rispetto a non accaparrarsi mai in fase di compilazione.

Quindi, sai come C # ha null ? Facciamo un'inversione di ciò e creiamo un Nullable<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); }
}

Ora, invece di scrivere

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

  // ...method code...
}

, scrivi solo:

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

Quindi, ci sono tre casi d'uso:

1. L'utente chiama NonNullable<T> con un% non coutente%:

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

   Questo è il caso d'uso desiderato e funziona con o senza Foo() .

2. L'utente chiama Stream con un NonNullable<> nullo:

   Stream stream = null;
   Foo(stream);
   

   Questo è un errore di chiamata. Qui Foo() aiuta a informare l'utente che non dovrebbero farlo, ma in realtà non li ferma. In entrambi i casi, questo risulta in un% di tempo di esecuzioneStream.

3. L'utente chiama NonNullable<> con NullArgumentException :

   Foo(null);
   

   Foo() non convertirà implicitamente in null , quindi l'utente riceve un errore nel tempo di esecuzione di IDE, prima . Si tratta di delegare il controllo nullo al sistema di tipi, proprio come lo raccomanderebbe l'SRP.

Puoi estendere questo metodo anche per affermare altre cose sui tuoi argomenti. Ad esempio, poiché desideri un flusso scrivibile, puoi definire un null che controlla sia NonNullable<> che struct WriteableStream<T> where T:Stream nel costruttore. Questo sarebbe comunque un controllo di tipo runtime, ma:

1. Decora il tipo con il qualificatore null , segnalando la necessità dei chiamanti.

2. Effettua il controllo in un'unica posizione nel codice, quindi non devi ripetere il controllo e stream.CanWrite ogni volta.

3. È più conforme all'SRP spingendo i compiti di controllo dei tipi al sistema di tipi (come esteso dai decoratori generici).

Il tuo approccio è attualmente procedurale. Stai distruggendo l'oggetto Stream e convalidandolo dall'esterno. Non farlo - rompe l'incapsulamento. Lascia che Stream sia responsabile della propria convalida. Non possiamo cercare di applicare l'SRP finché non avremo alcune classi per applicarlo a.

Ecco un Stream che esegue un'azione solo se supera la convalida:

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

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

Ma ora stiamo violando SRP! "Una classe dovrebbe avere solo una ragione per cambiare". Abbiamo un mix di 1) validazione e 2) logica effettiva. Abbiamo due motivi per cui potrebbe essere necessario cambiare.

Possiamo risolvere questo problema con convalidare i decoratori . Innanzitutto, dobbiamo convertire il nostro Stream in un'interfaccia e implementarlo come classe concreta.

interface Stream
{
    void someAction();
}

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

Ora possiamo scrivere un decoratore che racchiude un Stream , esegue la convalida e defener al dato Stream per la logica effettiva dell'azione.

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

Ora possiamo comporli come preferiamo:

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

Vuoi ulteriore convalida? Aggiungi un altro decoratore.

Il lavoro di una classe è fornire un servizio che soddisfa un contratto . Una classe ha sempre un contratto: una serie di requisiti per utilizzarlo e promette di farne il suo stato e le sue uscite purché i requisiti siano soddisfatti. Questo contratto può essere esplicito, tramite documentazione e / o asserzioni, o implicito, ma esiste sempre.

Una parte del contratto della tua classe è che il chiamante fornisce al costruttore alcuni argomenti che non devono essere nulli. L'implementazione del contratto è la responsabilità della classe, quindi per verificare che il chiamante abbia rispettato la sua parte del contratto può facilmente essere considerato nell'ambito della responsabilità della classe.

L'idea che una classe implementa un contratto è dovuta a Bertrand Meyer , il progettista del linguaggio di programmazione Eiffel e di l'idea di design per contratto . La lingua Eiffel rende le specifiche e il controllo della parte contrattuale della lingua.

Come è stato sottolineato in altre risposte, l'SRP è spesso frainteso. Non si tratta di avere il codice atomico che fa solo una funzione. Si tratta di assicurarsi che i tuoi oggetti e metodi facciano solo una cosa, e che l'unica cosa sia fatta in un solo posto.

Osserviamo un pessimo esempio in pseudo codice.

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

Nel nostro esempio piuttosto assurdo, la "responsabilità" del costruttore di Math # è rendere l'oggetto matematico utilizzabile. Lo fa innanzitutto sanitizzando l'input, quindi assicurandosi che i valori non siano -1.

Questo è SRP valido perché il costruttore sta facendo solo una cosa. Sta preparando l'oggetto Math. Tuttavia non è molto mantenibile. Si viola SECCO.

Quindi lasciaci fare un altro passaggio

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

In questo passaggio siamo un po 'meglio su DRY, ma abbiamo ancora un modo per andare con DRY. L'SRP d'altra parte sembra un po 'spento. Ora abbiamo due funzioni con lo stesso lavoro. Sia cleanX che cleany input.

Lascia fare un altro tentativo

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

Ora finalmente è meglio su DRY, e SRP sembra essere d'accordo. Abbiamo solo un posto che fa il lavoro "sanitizza".

Il codice è in teoria più gestibile e meglio ancora quando andiamo a correggere il bug e stringiamo il codice, dobbiamo solo farlo in un posto.

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

Nella maggior parte dei casi reali gli oggetti sarebbero più complessi e l'SRP sarebbe applicato su una serie di oggetti. Ad esempio, l'età può appartenere a Padre, Madre, Figlio, Figlia, quindi invece di avere 4 classi che identificano l'età dalla data di nascita, hai una classe Persona che lo fa e le 4 classi ereditano da quella. Ma spero che questo esempio aiuti a spiegare. SRP non si tratta di azioni atomiche, ma di un "lavoro" svolto.

Parlando di SRP, allo zio Bob non piacciono i controlli nulli sparsi ovunque. In generale tu, come una squadra, dovresti evitare di usare i parametri nulli nei costruttori quando possibile. Quando pubblichi il tuo codice al di fuori del tuo team, le cose potrebbero cambiare.

Applicare il non-nulla-valore dei parametri del costruttore senza prima assicurare la coesione della classe in questione risulta gonfiare il codice chiamante, specialmente i test.

Se vuoi davvero applicare questi contratti, considera l'utilizzo di Debug.Assert o qualcosa di simile per ridurre il disordine:

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

   // Go on normal operation.
}