Un constructeur qui valide ses arguments viole-t-il SRP?

J'essaie d'adhérer autant que possible au principe de responsabilité unique (SRP) et je me suis habitué à un certain modèle (pour le SRP sur les méthodes) qui repose fortement sur les délégués. J'aimerais savoir si cette approche est valable ou si elle pose de graves problèmes.

Par exemple, pour vérifier l'entrée dans un constructeur, je pourrais introduire la méthode suivante (l' Streamentrée est aléatoire, peut être n'importe quoi)

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

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

Cette méthode (sans doute) fait plus d'une chose

• Vérifiez les entrées
• Lancer différentes exceptions

Pour adhérer au SRP, j’ai donc changé la logique pour

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

Ce qui est censé ne faire qu’une chose (le fait-il?): Vérifiez l’entrée. Pour le contrôle effectif des entrées et la levée des exceptions, j'ai introduit des méthodes telles que

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

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

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

et peut appeler CheckInputcomme

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

Est-ce meilleur que la première option, ou est-ce que j'introduis une complexité inutile? Est-il possible d'améliorer encore ce modèle, s'il est viable?

SRP est peut-être le principe logiciel le plus mal compris.

Une application logicielle est construite à partir de modules, qui sont construits à partir de modules, qui sont construits à partir de ...

En bas, une seule fonction telle que CheckInputne contient qu'un tout petit peu de logique, mais à mesure que vous montez, chaque module successif encapsule de plus en plus de logique , ce qui est normal .

SRP ne consiste pas à faire une seule action atomique . Il s'agit d'avoir une seule responsabilité, même si cette responsabilité nécessite plusieurs actions ... et finalement, il s'agit de maintenance et de testabilité :

• il favorise l'encapsulation (éviter les objets divins),
• il favorise la séparation des problèmes (en évitant les modifications en chaîne dans l'ensemble du code),
• cela facilite la testabilité en réduisant l'étendue des responsabilités.

Le fait qu'il CheckInputsoit mis en œuvre avec deux contrôles et soulève deux exceptions différentes est sans importance dans une certaine mesure.

CheckInputa une responsabilité étroite: s'assurer que l'entrée est conforme aux exigences. Oui, il y a plusieurs exigences, mais cela ne signifie pas qu'il y a plusieurs responsabilités. Oui, vous pouvez diviser les chèques, mais comment cela aiderait-il? À un moment donné, les chèques doivent être répertoriés d’une manière ou d’une autre.

Comparons:

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

contre:

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

Maintenant, CheckInputfait moins ... mais son appelant fait plus!

Vous avez déplacé la liste des exigences de CheckInput, où elles sont encapsulées, à Constructoroù elles sont visibles.

Est-ce un bon changement? Ça dépend:

• Si CheckInputseulement appelé ici: c'est discutable, d'une part, il rend les exigences visibles, d'autre part, il encombre le code;
• Si CheckInputest appelé plusieurs fois avec les mêmes exigences , il viole DRY et vous avez un problème d'encapsulation.

Il est important de réaliser qu'une seule responsabilité peut impliquer beaucoup de travail. Le "cerveau" d'une voiture autonome a une seule responsabilité:

Conduire la voiture à sa destination.

C'est une responsabilité unique, mais il faut coordonner une tonne de capteurs et d'acteurs, prendre de nombreuses décisions, et même éventuellement satisfaire à des exigences contradictoires ¹ ...

... Cependant, tout est encapsulé. Donc, le client s'en fiche.

¹ sécurité des passagers, sécurité des autres, respect de la réglementation, ...

Citant Oncle Bob à propos du SRP ( https://8thlight.com/blog/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html ):

Le principe de responsabilité unique (SRP) stipule que chaque module logiciel doit avoir une et une seule raison de changer.

... Ce principe concerne les gens.

... Lorsque vous écrivez un module logiciel, vous voulez vous assurer que, lorsque des modifications sont demandées, ces modifications ne peuvent émaner que d'une seule personne, ou plutôt d'un seul groupe de personnes étroitement liées représentant une seule fonction métier étroitement définie.

... C'est la raison pour laquelle nous ne mettons pas de code SQL dans les fichiers JSP. C’est la raison pour laquelle nous ne générons pas de code HTML dans les modules qui calculent les résultats. C'est la raison pour laquelle les règles métier ne doivent pas connaître le schéma de base de données. C'est la raison pour laquelle nous séparons les préoccupations.

Il explique que les modules logiciels doivent répondre aux préoccupations spécifiques des parties prenantes. Donc, répondant à votre question:

Est-ce meilleur que la première option, ou est-ce que j'introduis une complexité inutile? Est-il possible d'améliorer encore ce modèle, s'il est viable?

OMI, vous ne considérez qu'une méthode lorsque vous devez rechercher un niveau supérieur (niveau de classe dans ce cas). Nous devrions peut-être jeter un coup d'œil sur ce que fait actuellement votre classe (et cela nécessite davantage d'explications sur votre scénario). Pour le moment, votre classe fait toujours la même chose. Par exemple, si demain il y a une demande de changement concernant une validation (par exemple: "le flux peut maintenant être nul"), vous devez toujours accéder à cette classe et y modifier les éléments qu'elle contient.

Non, cette modification n’est pas informée par le PÉR.

Demandez-vous pourquoi il n'y a pas de vérification dans votre vérificateur pour "l'objet transmis est un flux" . La réponse est évidente: le langage empêche l'appelant de compiler un programme qui passe dans un flux autre que le flux.

Le système de types de C # est insuffisant pour répondre à vos besoins; vos chèques implémentent l'application d'invariants qui ne peuvent pas être exprimés dans le système de types aujourd'hui . S'il y avait un moyen de dire que la méthode utilise un flux inscriptible non nullable, vous l'auriez écrit, mais ce n'est pas le cas. Vous avez donc opté pour la meilleure solution: vous avez appliqué la restriction de type au moment de l'exécution. J'espère que vous l'avez également documentée, afin que les développeurs qui utilisent votre méthode n'aient pas à la violer, à faire échouer leurs scénarios de test, puis à résoudre le problème.

Mettre des types sur une méthode n'est pas une violation du principe de responsabilité unique; la méthode n'applique pas non plus ses conditions préalables ni ses post-conditions.

Toutes les responsabilités ne sont pas égales.

Voici deux tiroirs. Ils ont tous deux une responsabilité. Ils ont chacun des noms qui vous permettent de savoir ce qui leur appartient. L'un est le tiroir à couverts. L'autre est le tiroir à ordures.

Alors quelle est la différence? Le tiroir à couverts indique clairement ce qui ne lui appartient pas. Le tiroir indésirable accepte cependant tout ce qui conviendra. Sortir les cuillères du tiroir à couverts semble très faux. Pourtant, j'ai bien du mal à penser à tout ce qui nous manquerait s'il était retiré du tiroir pour ordures. La vérité est que vous pouvez affirmer que n'importe quoi a une responsabilité unique, mais selon vous, quelle est la responsabilité individuelle la plus ciblée?

Un objet ayant une seule responsabilité ne signifie pas qu'une seule chose peut se produire ici. Les responsabilités peuvent nid. Mais ces responsabilités de nidification devraient avoir un sens, elles ne devraient pas vous surprendre quand vous les trouverez ici et vous devriez les manquer si elles étaient parties.

Alors quand tu offres

CheckInput(Stream stream);

Je ne me trouve pas préoccupé par le fait qu’il est à la fois de vérifier les entrées et de lever des exceptions. Je serais inquiet s'il s'agissait à la fois de vérifier l'entrée et de sauvegarder l'entrée. C'est une mauvaise surprise. Un que je ne manquerais pas s'il était parti.

Lorsque vous vous nouez et écrivez un code étrange afin de vous conformer à un principe important du logiciel, vous avez généralement mal compris le principe (même si, parfois, le principe est erroné). Comme le souligne l'excellente réponse de Matthieu, toute la signification de SRP dépend de la définition de "responsabilité".

Les programmeurs expérimentés voient ces principes et les relient à des mémoires de code que nous avons bousillés; les programmeurs moins expérimentés les voient et n'ont peut-être rien à voir avec eux. C'est une abstraction qui flotte dans l'espace, tout sourire et pas de chat. Alors ils devinent, et ça va généralement mal. Avant que vous ayez développé le sens du langage de programmation, la différence entre un code bizarre et trop compliqué et un code normal n’est pas évidente.

Ce n'est pas un commandement religieux auquel vous devez obéir quelles que soient les conséquences personnelles. Il s’agit plus d’une règle empirique formelle qui vise à formaliser un élément de la programmation sensorielle et à vous aider à garder votre code aussi simple et clair que possible. Si cela a l'effet opposé, vous avez raison de rechercher une entrée extérieure.

En programmation, vous ne pouvez pas vous tromper beaucoup d’essayer de déduire la signification d’un identifiant à partir de principes premiers en le fixant, et cela vaut pour les identifiants écrits autant sur la programmation que les identifiants dans le code réel.

Rôle CheckInput

Tout d’abord, permettez-moi d’énoncer une évidence, CheckInput c’est faire une chose, même si elle vérifie divers aspects. En fin de compte, il vérifie les entrées . On pourrait dire que ce n’est pas une chose si vous utilisez des méthodes appelées DoSomething, mais je pense qu’il est prudent de supposer que la vérification des entrées n’est pas trop vague.

L'ajout de ce modèle pour les prédicats peut être utile si vous souhaitez que la logique de contrôle de l'entrée soit placée dans votre classe, mais que ce modèle semble plutôt détaillé pour ce que vous essayez d'atteindre. Il pourrait être beaucoup plus direct de simplement passer une interface IStreamValidatoravec une méthode unique isValid(Stream)si c'est ce que vous souhaitez obtenir. Toute classe implémentée IStreamValidatorpeut utiliser des prédicats comme StreamIsNullou StreamIsReadonlysi elle le souhaite, mais pour en revenir au point central, il est plutôt ridicule de procéder à un changement afin de préserver le principe de responsabilité unique.

Verification sanitaire

Je pense que nous sommes tous autorisés à effectuer un "contrôle d'intégrité" afin de nous assurer que vous traitez au moins avec un flux non nul et inscriptible. Ce contrôle de base ne fait en aucun cas de votre classe un validateur de flux. Remarquez, il serait préférable de laisser des chèques plus sophistiqués en dehors de votre classe, mais c'est là que la ligne est tracée. Une fois que vous avez besoin de commencer à modifier l'état de votre flux en le lisant ou en consacrant des ressources à la validation, vous avez commencé à effectuer une validation formelle de votre flux. C'est ce qu'il convient de transférer dans sa propre classe.

Conclusion

Mes pensées sont que si vous appliquez un modèle pour mieux organiser un aspect de votre classe, cela mérite de figurer dans sa propre classe. Puisqu'un motif ne correspond pas, vous devez également vous demander s'il appartient ou non à sa propre classe. Je pense que, sauf si vous pensez que la validation du flux va probablement être modifiée à l'avenir, et particulièrement si vous pensez que cette validation peut même être de nature dynamique, le modèle que vous avez décrit est une bonne idée, même si être d'abord trivial. Sinon, il n'est pas nécessaire de rendre votre programme plus complexe de manière arbitraire. Permet d'appeler un chat un chat. La validation est une chose, mais vérifier l’entrée nulle n’est pas une validation. Par conséquent, je pense que vous pouvez être sûr de la conserver dans votre classe sans violer le principe de responsabilité unique.

Le principe n'énonce pas catégoriquement qu'un morceau de code ne devrait "faire qu'une seule chose".

La "responsabilité" dans SRP doit être comprise au niveau des exigences. La responsabilité du code est de satisfaire les exigences commerciales. SRP est violé si un objet satisfait à plusieurs exigences commerciales indépendantes . Indépendant, cela signifie qu'une exigence peut changer tandis que l'autre exigence reste en place.

Il est concevable qu'une nouvelle exigence métier soit introduite, ce qui signifie que cet objet particulier ne doit pas vérifier la lisibilité, alors qu'une autre exigence métier exige toujours que l'objet vérifie si elle est lisible. Non, car les exigences métier ne spécifient pas les détails de la mise en œuvre à ce niveau.

Un exemple concret de violation de SRP serait un code comme celui-ci:

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

Ce code est très simple, mais il est néanmoins concevable que le texte change indépendamment de la date de livraison prévue, car celles-ci sont décidées par différentes parties de l'entreprise.

J'aime le point de la réponse de @ EricLippert :

Demandez-vous pourquoi il n'y a pas de vérification dans votre vérificateur si l'objet transmis est un flux . La réponse est évidente: le langage empêche l'appelant de compiler un programme qui passe dans un flux autre que le flux.

Le système de types de C # est insuffisant pour répondre à vos besoins; vos chèques implémentent l'application d'invariants qui ne peuvent pas être exprimés dans le système de types aujourd'hui . S'il y avait un moyen de dire que la méthode utilise un flux inscriptible non nullable, vous l'auriez écrit, mais ce n'est pas le cas. Vous avez donc opté pour la meilleure solution: vous avez appliqué la restriction de type au moment de l'exécution. J'espère que vous l'avez également documentée, afin que les développeurs qui utilisent votre méthode n'aient pas à la violer, à faire échouer leurs scénarios de test, puis à résoudre le problème.

EricLippert a raison de dire que c'est un problème pour le système de types. Et puisque vous voulez utiliser le principe de responsabilité unique (SRP), vous avez essentiellement besoin que le système de types soit responsable de ce travail.

Il est en fait possible de le faire en C #. Nous pouvons capturer des littéraux nullau moment de la compilation, puis des non littéraux nullau moment de l'exécution. Ce n'est pas aussi bon qu'un contrôle complet à la compilation, mais c'est une amélioration stricte par rapport à ne jamais attraper au moment de la compilation.

Alors, tu sais comment C # a Nullable<T>? Inversons cela et faisons un 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); }
}

Maintenant, au lieu d'écrire

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

  // ...method code...
}

, Ecrivez:

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

Ensuite, il y a trois cas d'utilisation:

1. Appels de l'utilisateur Foo()avec un non-null Stream:

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

   C'est le cas d'utilisation souhaité, et cela fonctionne avec ou sans NonNullable<>.

2. Les appels de l'utilisateur Foo()avec un null Stream:

   Stream stream = null;
   Foo(stream);

   Ceci est une erreur d'appel. Ici, on NonNullable<>informe l'utilisateur qu'il ne devrait pas le faire, mais cela ne les empêche pas vraiment. Dans les deux cas, cela entraîne une exécution NullArgumentException.

3. Appels de l'utilisateur Foo()avec null:

   Foo(null);

   nullne se convertira pas implicitement en a NonNullable<>, de sorte que l'utilisateur obtient une erreur dans l'EDI avant l' exécution. Cela consiste à déléguer la vérification de nullité au système de types, comme le conseillerait la SRP.

Vous pouvez étendre cette méthode pour affirmer d'autres choses à propos de vos arguments. Par exemple, puisque vous voulez un flux accessible en écriture, vous pouvez définir un struct WriteableStream<T> where T:Streamcontrôle à la fois pour le constructeur nullet stream.CanWritepour celui-ci. Ce serait toujours une vérification de type à l'exécution, mais:

1. Il décore le type avec le WriteableStreamqualificatif, signalant le besoin aux appelants.

2. Il effectue la vérification à un seul endroit dans le code, de sorte que vous n'avez pas à répéter la vérification et à throw InvalidArgumentExceptionchaque fois.

3. Il se conforme mieux à la SRP en transférant les tâches de vérification de type vers le système de types (tel qu'étendu par les décorateurs génériques).

Votre approche est actuellement procédurale. Vous brisez l' Streamobjet et le validez de l'extérieur. Ne faites pas ça - ça casse l'encapsulation. Laissez le Streamresponsable de sa propre validation. Nous ne pouvons pas chercher à appliquer le PÉR tant que nous n’aurons pas quelques classes pour l’appliquer.

Voici Streamune action qui effectue une action uniquement si elle réussit la validation:

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

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

Mais maintenant nous violons SRP! "Une classe ne devrait avoir qu'une seule raison de changer." Nous avons un mélange de 1) validation et 2) de logique réelle. Nous avons deux raisons pour lesquelles il pourrait être nécessaire de changer.

Nous pouvons résoudre ce problème en validant les décorateurs . Premièrement, nous devons convertir notre Streaminterface en interface et la mettre en œuvre sous forme de classe concrète.

interface Stream
{
    void someAction();
}

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

Nous pouvons maintenant écrire un décorateur qui enveloppe a Stream, effectue la validation et reporte à la donnée Streampour la logique réelle de l'action.

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

Nous pouvons maintenant les composer comme bon nous semble:

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

Voulez-vous une validation supplémentaire? Ajouter un autre décorateur.

Le travail d'une classe consiste à fournir un service conforme à un contrat . Une classe a toujours un contrat: un ensemble d’exigences pour l’utiliser et des promesses qu’elle fait sur son état et ses résultats à condition que les exigences soient remplies. Ce contrat peut être explicite, sous forme de documentation et / ou d’assertions, ou implicite, mais il existe toujours.

Une partie du contrat de votre classe est que l'appelant donne au constructeur des arguments qui ne doivent pas être nuls. La mise en œuvre du contrat est la responsabilité de la classe, donc vérifier que l'appelant a rempli sa part du contrat peut facilement être considérée comme relevant de la responsabilité de la classe.

Bertrand Meyer , concepteur du langage de programmation Eiffel et concept du design par contrat, est à l'origine de la conclusion d'un contrat par une classe . Le langage Eiffel fait de la spécification et de la vérification du contrat une partie de la langue.

Comme cela a été souligné dans d'autres réponses, le PRS est souvent mal compris. Il ne s'agit pas d'avoir du code atomique qui ne remplit qu'une fonction. Il s'agit de s'assurer que vos objets et méthodes ne font qu'une seule chose, et que la seule chose soit faite au même endroit.

Regardons un mauvais exemple en pseudo-code.

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

Dans notre exemple plutôt absurde, la "responsabilité" du constructeur Math # est de rendre l'objet mathématique utilisable. Cela se fait d'abord en nettoyant l'entrée, puis en s'assurant que les valeurs ne sont pas -1.

C'est un SRP valide car le constructeur ne fait qu'une chose. C'est préparer l'objet Math. Cependant ce n'est pas très maintenable. Il viole DRY.

Prenons donc une autre passe

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

Dans cette passe, nous avons un peu mieux travaillé sur DRY, mais nous avons encore du chemin à faire avec DRY. La SRP par contre semble un peu en retrait. Nous avons maintenant deux fonctions avec le même travail. CleanX et cleanY nettoient les entrées.

Permet de lui donner un autre coup

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

Maintenant, nous étions enfin mieux à propos de DRY, et SRP semble être d’accord. Nous n'avons qu'un seul endroit qui fait le travail "d'assainissement".

Le code est théoriquement plus facile à gérer et à améliorer, même lorsque nous corrigeons le bogue et le resserrions, nous n'avons besoin de le faire qu’à un seul endroit.

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

Dans la plupart des cas réels, les objets seraient plus complexes et la SRP serait appliquée à un groupe d'objets. Par exemple, age peut appartenir à Père, Mère, Fils, Fille. Ainsi, au lieu d’avoir 4 classes qui calculent l’âge à partir de la date de naissance, vous avez une classe Personne qui fait cela et les 4 classes en héritent. Mais j'espère que cet exemple aide à expliquer. SRP ne concerne pas les actions atomiques, mais un "travail" en train de se faire.

En parlant de SRP, oncle Bob n'aime pas les chèques nuls éparpillés partout. En général, vous, en tant qu'équipe, devriez éviter d'utiliser des paramètres nuls pour les constructeurs dans la mesure du possible. Lorsque vous publiez votre code en dehors de votre équipe, les choses peuvent changer.

L'application de la non-nullabilité des paramètres du constructeur sans d'abord assurer la cohésion de la classe en question entraîne une surcharge du code appelant, notamment des tests.

Si vous souhaitez vraiment faire respecter de tels contrats, envisagez d'utiliser un logiciel Debug.Assertsimilaire pour réduire l'encombrement:

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

   // Go on normal operation.
}