Applicabilité du principe de responsabilité unique

Je suis récemment tombé sur un problème architectural apparemment trivial. J'ai eu un dépôt simple dans mon code qui s'appelait comme ceci (le code est en C #):

var user = /* create user somehow */;
_userRepository.Add(user);
/* do some other stuff*/
_userRepository.SaveChanges();

SaveChanges était un simple wrapper qui valide les modifications dans la base de données:

void SaveChanges()
{
    _dataContext.SaveChanges();
    _logger.Log("User DB updated: " + someImportantInfo);
}

Ensuite, après un certain temps, je devais mettre en œuvre une nouvelle logique permettant d'envoyer des notifications par courrier électronique chaque fois qu'un utilisateur était créé dans le système. Comme il y a eu beaucoup d'appels vers _userRepository.Add()et SaveChangesautour du système, j'ai décidé de mettre à jour de la manière SaveChangessuivante:

void SaveChanges()
{
    _dataContext.SaveChanges();
    _logger.Log("User DB updated: " + someImportantInfo);
    foreach (var newUser in dataContext.GetAddedUsers())
    {
       _eventService.RaiseEvent(new UserCreatedEvent(newUser ))
    }
}

De cette manière, du code externe pourrait s'abonner à UserCreatedEvent et gérer la logique métier nécessaire pour envoyer des notifications.

Mais on m'a fait remarquer que ma modification de SaveChangesviolait le principe de responsabilité unique et que cela SaveChangesne devrait que sauver et ne déclencher aucun événement.

Est-ce un point valable? Il me semble que la création d'un événement ici est essentiellement la même chose que la journalisation: il suffit d'ajouter une fonctionnalité latérale à la fonction. Et SRP ne vous interdit pas d'utiliser des événements de journalisation ou de déclenchement dans vos fonctions, il indique simplement qu'une telle logique doit être encapsulée dans d'autres classes et qu'il est correct qu'un référentiel appelle ces autres classes.

Oui, il peut être une exigence valable d'avoir un référentiel qui déclenche certains événements sur certaines actions comme Addou SaveChanges- et je ne vais pas remettre en question cette (comme d'autres réponses) juste parce que votre exemple spécifique de l' ajout d' utilisateurs et d' envoyer des e - mails peut sembler un peu artificiel. Dans la suite, supposons que cette exigence est parfaitement justifiée dans le contexte de votre système.

Alors oui , coder la mécanique des événements, ainsi que la journalisation et l'enregistrement dans une méthode est une violation de la SRP . Dans de nombreux cas, il s'agit probablement d'une violation acceptable, en particulier lorsque personne ne souhaite jamais répartir les responsabilités de maintenance "Enregistrer les modifications" et "Relancer l'événement" entre différentes équipes / responsables. Mais supposons un jour que quelqu'un veuille faire exactement cela, cela peut-il être résolu de manière simple, peut-être en mettant le code de ces problèmes dans différentes bibliothèques de classes?

La solution consiste à laisser votre référentiel d'origine rester responsable des modifications apportées à la base de données, rien de plus, et créer un référentiel proxy qui possède exactement la même interface publique, réutilise le référentiel d'origine et ajoute les mécanismes d'événement supplémentaires aux méthodes.

// In EventFiringUserRepo:
public void SaveChanges()
{
  _basicRepo.SaveChanges();
   FireEventsForNewlyAddedUsers();
}

private void FireEventsForNewlyAddedUsers()
{
  foreach (var newUser in _basicRepo.DataContext.GetAddedUsers())
  {
     _eventService.RaiseEvent(new UserCreatedEvent(newUser))
  }
}

Vous pouvez appeler la classe de proxy a NotifyingRepositoryou ObservableRepositorysi vous le souhaitez, dans le sens de la réponse hautement votée de @ Peter (qui ne dit en fait pas comment résoudre la violation SRP, mais indique simplement que la violation est correcte).

La nouvelle et l'ancienne classe de référentiel doivent toutes deux dériver d'une interface commune, comme indiqué dans la description de modèle de proxy classique .

Puis, dans votre code d'origine, initialisez _userRepositorypar un objet de la nouvelle EventFiringUserRepoclasse. De cette façon, vous gardez le référentiel d'origine séparé de la mécanique des événements. Si nécessaire, vous pouvez placer côte à côte le référentiel de déclenchement d'événements et le référentiel d'origine et laisser les appelants décider s'ils utilisent l'un ou l'autre.

Pour répondre à une préoccupation mentionnée dans les commentaires: cela ne mène-t-il pas à des procurations en plus de procurations à des procurations, et ainsi de suite? En fait, l'ajout de mécanismes d'événements crée une base permettant d'ajouter d'autres exigences du type "envoyer des courriels" en souscrivant simplement aux événements. Vous devez donc vous en tenir au SRP avec ces exigences, sans autre mandataire. Mais la seule chose à ajouter une fois est la mécanique d’événement elle-même.

Si ce type de séparation en vaut vraiment la peine dans le contexte de votre système, vous et votre relecteur devez en décider vous-même. Je ne voudrais probablement pas séparer la journalisation du code d'origine, ni en utilisant un autre proxy, ni en ajoutant un consignateur à l'événement d'écoute, même si cela serait possible.

L'envoi d'une notification indiquant que le magasin de données persistantes a été modifié semble être une bonne chose à faire lors de la sauvegarde.

Bien entendu, vous ne devez pas considérer Add comme un cas spécial - vous devez également déclencher des événements pour Modify et Delete. C'est le traitement spécial du cas "Ajouter" qui sent, oblige le lecteur à expliquer pourquoi il sent, et amène finalement certains lecteurs du code à conclure qu'il doit violer le PÉR.

Un référentiel "notifiant" qui peut être interrogé, modifié et déclenche des événements lors de modifications est un objet parfaitement normal. Vous pouvez vous attendre à en trouver plusieurs variantes dans presque tous les projets de taille décente.

Mais un référentiel "notifiant" est-il réellement ce dont vous avez besoin? Vous avez mentionné C #: de nombreuses personnes s'accorderaient pour dire que l'utilisation de a System.Collections.ObjectModel.ObservableCollection<>au lieu de System.Collections.Generic.List<>lorsque ce dernier est tout ce dont vous avez besoin est une panoplie de mauvaises et de mauvaises erreurs, mais peu de gens se dirigent immédiatement vers SRP.

Ce que vous faites maintenant, c'est échanger votre UserList _userRepositoryavec un ObservableUserCollection _userRepository. Si tel est le meilleur plan d'action ou non dépend de l'application. Mais bien que cela rend incontestablement le _userRepositorypoids considérablement moins léger, à mon humble avis, il ne viole pas le PÉR.

Oui, c'est une violation du principe de responsabilité unique et un argument valable.

Une meilleure conception consisterait à avoir un processus séparé pour extraire les «nouveaux utilisateurs» du référentiel et envoyer les courriels. Garder une trace des utilisateurs qui ont reçu un email, des échecs, des renvois, etc., etc.

De cette façon, vous pouvez gérer les erreurs, les plantages, etc., tout en évitant que votre référentiel ne saisisse toutes les exigences, ce qui laisse penser que des événements se produisent "lorsqu'un élément est validé dans la base de données".

Le référentiel ne sait pas qu'un utilisateur que vous ajoutez est un nouvel utilisateur. Sa responsabilité consiste simplement à stocker l'utilisateur.

Cela vaut probablement la peine de développer les commentaires ci-dessous.

Le référentiel ne sait pas qu'un utilisateur que vous ajoutez est un nouvel utilisateur. Oui, il dispose d'une méthode appelée Ajouter. Sa sémantique implique que tous les utilisateurs ajoutés sont de nouveaux utilisateurs. Combinez tous les arguments passés à Ajouter avant d'appeler Enregistrer - et vous obtenez tous les nouveaux utilisateurs

Incorrect. Vous associez "Ajouté au référentiel" et "Nouveau".

"Ajouté au référentiel" signifie exactement ce qu'il dit. Je peux ajouter et supprimer et ré-ajouter des utilisateurs à différents référentiels.

"Nouveau" est un état d'utilisateur défini par des règles commerciales.

Actuellement, la règle de gestion peut être "Le nouveau = = vient d'être ajouté au référentiel", mais cela ne signifie pas que ce n'est pas une responsabilité distincte de connaître et d'appliquer cette règle.

Vous devez faire attention à éviter ce genre de pensée centrée sur la base de données. Vous aurez des processus de cas marginaux qui ajoutent des utilisateurs non-nouveaux au référentiel et lorsque vous leur envoyez des courriels, toute l'entreprise dira "Bien sûr, ce ne sont pas de" nouveaux "utilisateurs! La règle réelle est X".

Cette réponse est IMHO manque vraiment le point: le repo est exactement la place centrale dans le code qui sait quand de nouveaux utilisateurs sont ajoutés

Incorrect. Pour les raisons ci-dessus, en outre, il ne s'agit pas d'un emplacement central, sauf si vous incluez réellement le code d'envoi du courrier électronique dans la classe plutôt que de simplement déclencher un événement.

Vous aurez des applications qui utilisent la classe de référentiel, mais vous n'avez pas le code pour envoyer l'e-mail. Lorsque vous ajoutez des utilisateurs à ces applications, le courrier électronique ne sera pas envoyé.

Est-ce un point valable?

Oui, bien que cela dépende beaucoup de la structure de votre code. Je n'ai pas le contexte complet alors je vais essayer de parler en général.

Il me semble que la création d'un événement ici est essentiellement la même chose que la journalisation: il suffit d'ajouter une fonctionnalité latérale à la fonction.

Ce n'est absolument pas. La journalisation ne fait pas partie du flux commercial, elle peut être désactivée, ne doit pas causer d’effets secondaires (commerciaux) et ne doit en aucune manière influer sur l’état et la santé de votre application, même si, pour une raison quelconque, vous ne pouviez pas vous connecter. plus rien. Maintenant, comparez cela avec la logique que vous avez ajoutée.

Et SRP ne vous interdit pas d'utiliser des événements de journalisation ou de déclenchement dans vos fonctions, il indique simplement qu'une telle logique doit être encapsulée dans d'autres classes et qu'il est correct qu'un référentiel appelle ces autres classes.

SRP fonctionne en tandem avec ISP (S et I dans SOLID). Vous vous retrouvez avec de nombreuses classes et méthodes qui font des choses très spécifiques et rien d’autre. Ils sont très ciblés, très faciles à mettre à jour ou à remplacer et, en général, plus faciles à tester. Bien entendu, dans la pratique, vous aurez également quelques classes plus importantes qui traitent de l’orchestration: elles auront un certain nombre de dépendances et ne se concentreront pas sur des actions atomisées, mais sur des actions commerciales, qui peuvent nécessiter plusieurs étapes. Tant que le contexte commercial est clair, ils peuvent également être qualifiés de responsabilité unique, mais comme vous l'avez dit à juste titre, à mesure que le code évolue, vous souhaiterez peut-être en extraire une partie dans de nouvelles classes / interfaces.

Revenons maintenant à votre exemple particulier. Si vous devez absolument envoyer une notification à chaque fois qu'un utilisateur est créé et peut-être même effectuer d'autres actions plus spécialisées, vous pouvez créer un service séparé qui encapsule cette exigence, comme par exemple UserCreationService, qui expose une méthode Add(user), qui gère à la fois le stockage (l'appel dans votre référentiel) et la notification en une seule action commerciale. Ou faites-le dans votre extrait original, après_userRepository.SaveChanges();

En théorie, SRP concerne les gens , comme Oncle Bob l'explique dans son article Le principe de la responsabilité unique . _{Merci Robert Harvey pour le fournir dans votre commentaire.}

La bonne question est:

Quelle "partie prenante" a ajouté l'exigence "envoyer des courriels"?

Si cette partie prenante est également responsable de la persistance des données (improbable mais possible), cela ne constitue pas une violation du PRS. Sinon, c'est le cas.

Bien que, techniquement, les référentiels notifiant des événements soient parfaits, je suggérerais de le regarder d'un point de vue fonctionnel lorsque sa commodité soulève certaines préoccupations.

Créer un utilisateur, décider de ce qu’est un nouvel utilisateur et de sa persistance sont 3 choses différentes .

^{Mon local}

Prenez en compte le principe précédent avant de décider si le référentiel est le lieu approprié pour notifier les événements métier (quel que soit le PRS). Notez que j'ai dit événement commercial parce que pour moi UserCreateda une connotation différente de UserStoredou UserAdded ¹ . Je considérerais également que chacun de ces événements s'adresse à des publics différents.

D'un côté, la création d'utilisateurs est une règle spécifique à l'entreprise qui peut impliquer ou non la persistance. Cela pourrait impliquer davantage d'opérations commerciales, impliquant davantage d'opérations de base de données / réseau. Opérations ignorées par la couche de persistance. La couche de persistance n'a pas assez de contexte pour décider si le cas d'utilisation s'est terminé avec succès ou non.

D'un autre côté, ce n'est pas nécessairement vrai que _dataContext.SaveChanges();l'utilisateur a persisté. Cela dépendra de la durée de transaction de la base de données. Par exemple, cela pourrait être vrai pour des bases de données telles que MongoDB, dont les transactions sont atomiques, mais pas pour les SGBDR traditionnels mettant en œuvre des transactions ACID où davantage de transactions pourraient être impliquées et non encore validées.

Est-ce un point valable?

Il pourrait être. Cependant, j'oserais dire que ce n'est pas seulement une question de PRS (techniquement parlant), mais aussi de commodité (fonctionnellement parlant).

• Est-il commode de déclencher des événements commerciaux à partir de composants ignorant les opérations commerciales en cours?
• Représentent-ils le bon endroit autant que le bon moment pour le faire?
• Devrais-je autoriser ces composants à orchestrer ma logique métier au moyen de notifications comme celle-ci?
• Pourrais-je invalider les effets secondaires causés par des événements prématurés? ²

Il me semble que la relance d’un événement ici est essentiellement la même chose que l’exploitation forestière.

Absolument pas. La journalisation est censée n'avoir aucun effet secondaire, car vous avez suggéré que l'événement UserCreatedprovoquerait probablement d'autres opérations commerciales. Comme les notifications. ³

il dit simplement qu'une telle logique doit être encapsulée dans d'autres classes et qu'il est correct qu'un référentiel appelle ces autres classes

Pas nécessairement vrai. SRP n'est pas une préoccupation spécifique à la classe. Il fonctionne à différents niveaux d'abstractions, comme les couches, les bibliothèques et les systèmes! Il s'agit de cohésion, de garder ensemble ce qui change pour les mêmes raisons de la part des mêmes parties prenantes . Si la création de l'utilisateur ( cas d'utilisation ) change, c'est probablement le moment et les raisons pour lesquelles l'événement se produit changent également.

^{1: Nommer les choses de manière adéquate est également important.}

^{2: Disons que nous avons envoyé UserCreatedaprès _dataContext.SaveChanges();, mais que la transaction de base de données entière a échoué ultérieurement en raison de problèmes de connexion ou de violations de contraintes. Soyez prudent avec la diffusion prématurée des événements, car ses effets secondaires peuvent être difficiles à annuler (si cela est même possible).}

3: Les processus de notification non traités correctement peuvent vous amener à envoyer des notifications impossibles à annuler / supprimer>

Non, cela ne viole pas le PÉR.

Beaucoup semblent penser que le principe de responsabilité unique signifie qu'une fonction ne doit faire «qu'une chose», puis se laisser prendre dans la discussion sur ce qui constitue «une chose».

Mais ce n’est pas ce que signifie le principe. Il s'agit de préoccupations au niveau des entreprises. Une classe ne doit pas implémenter plusieurs préoccupations ou exigences pouvant changer indépendamment au niveau de l'entreprise. Disons qu'une classe à la fois stocke l'utilisateur et envoie un message de bienvenue codé en dur par courrier électronique. Plusieurs préoccupations indépendantes pourraient entraîner le changement des exigences d’une telle classe. Le concepteur peut exiger que le code HTML / feuille de style du courrier soit modifié. L’expert en communication pourrait exiger que le libellé du courrier soit modifié. Et l'expert UX pourrait décider que le courrier devrait être envoyé à un point différent du flux d'intégration. La classe est donc sujette à de nombreuses modifications d'exigences provenant de sources indépendantes. Cela viole le SRP.

Mais déclencher un événement ne constitue pas une violation de la SRP, car il ne dépend que de la sauvegarde de l'utilisateur et non d'une autre préoccupation. Les événements sont en fait un très bon moyen de respecter le SRP, car vous pouvez avoir un courrier électronique déclenché par la sauvegarde sans que le référentiel soit affecté par le courrier, ou même qu'il en soit informé.

Ne vous inquiétez pas du principe de responsabilité unique. Cela ne vous aidera pas à prendre une bonne décision ici, car vous pouvez choisir subjectivement un concept particulier en tant que "responsabilité". Vous pouvez dire que la classe est responsable de la gestion de la persistance des données dans la base de données ou que sa responsabilité consiste à effectuer tout le travail lié à la création d'un utilisateur. Ce ne sont que différents niveaux de comportement de l'application, et ce sont deux expressions conceptuelles valables d'une "responsabilité unique". Donc, ce principe n'est pas utile pour résoudre votre problème.

Le principe le plus utile à appliquer dans ce cas est le principe de la moindre surprise . Alors posons la question suivante: est-il étonnant qu’un référentiel ayant pour rôle principal de conserver des données dans une base de données envoie également des courriers électroniques?

Oui, c'est très surprenant. Ce sont deux systèmes externes complètement séparés, et le nom SaveChangesn'implique pas l'envoi de notifications. Le fait que vous déléguiez ceci à un événement rend le comportement encore plus surprenant, car une personne qui lit le code ne peut plus facilement voir quels comportements supplémentaires sont invoqués. L'indirection nuit à la lisibilité. Parfois, les avantages sont dignes des coûts de lisibilité, mais pas lorsque vous appelez automatiquement un système externe supplémentaire ayant des effets observables pour les utilisateurs finaux. (La journalisation peut être exclue ici car son effet consiste essentiellement à conserver des enregistrements à des fins de débogage. Les utilisateurs finaux ne consomment pas le journal, il n'y a donc aucun inconvénient à ce que la journalisation soit toujours effectuée.) Pire encore, cela réduit la flexibilité du minutage. d’envoyer le courrier électronique, rendant impossible l’entrelacement d’autres opérations entre la sauvegarde et la notification.

Si votre code doit généralement envoyer une notification lorsqu'un utilisateur est créé avec succès, vous pouvez créer une méthode pour le faire:

public void AddUserAndNotify(IUserRepository repo, IEmailNotification notifier, MyUser user)
{
    repo.Add(user);
    repo.SaveChanges();
    notifier.SendUserCreatedNotification(user);
}

Mais si cela ajoute de la valeur dépend des spécificités de votre application.

En fait, je découragerais l'existence de la SaveChangesméthode. Cette méthode va probablement valider une transaction de base de données, mais d' autres référentiels peuvent avoir modifié la base de données dans la même transaction . Le fait qu'il les engage toutes est à nouveau surprenant, car il SaveChangesest spécifiquement lié à cette instance du référentiel d'utilisateurs.

Le modèle le plus simple pour gérer une transaction de base de données est un usingbloc externe :

using (DataContext context = new DataContext())
{
    _userRepository.Add(context, user);
    context.SaveChanges();
    notifier.SendUserCreatedNotification(user);
}

Cela donne au programmeur un contrôle explicite sur le moment où les modifications de tous les référentiels sont enregistrées, oblige le code à documenter explicitement la séquence d'événements devant se produire avant une validation, garantit qu'une annulation est émise en cas d'erreur (en supposant qu'une annulation est émise DataContext.Dispose) et évite les occultations. les connexions entre les classes avec état.

Je préférerais également ne pas envoyer l'e-mail directement dans la demande. Il serait plus robuste d’enregistrer le besoin d’une notification dans une file d’attente. Cela permettrait une meilleure gestion des échecs. En particulier, si une erreur survient lors de l'envoi du courrier électronique, il peut être réessayé ultérieurement sans interrompre la sauvegarde de l'utilisateur. Cela évite le cas où l'utilisateur est créé mais une erreur est renvoyée par le site.

using (DataContext context = new DataContext())
{
    _userRepository.Add(context, user);
    _emailNotificationQueue.AddUserCreateNotification(user);
    _emailNotificationQueue.Commit();
    context.SaveChanges();
}

Il est préférable de valider d'abord la file d'attente de notification, car le consommateur de la file d'attente peut vérifier que l'utilisateur existe avant d'envoyer le courrier électronique, en cas context.SaveChanges()d'échec de l' appel. (Sinon, vous aurez besoin d'une stratégie de validation complète en deux phases pour éviter les heisenbugs.)

L'essentiel est d'être pratique. Réfléchissez réellement aux conséquences (à la fois en termes de risque et d’avantages) de l’écriture de code d’une manière particulière. Je trouve que le "principe de responsabilité unique" ne m'aide pas très souvent à faire cela, alors que le "principe de moindre surprise" m'aide souvent à entrer dans la tête d'un autre développeur (pour ainsi dire) et à réfléchir à ce qui pourrait arriver.

Actuellement, il SaveChangesfait deux choses: il enregistre les modifications et enregistre les résultats. Maintenant, vous voulez ajouter quelque chose: envoyer des notifications par courrier électronique.

Vous avez eu l’idée intelligente d’y ajouter un événement, mais celui-ci a été critiqué pour avoir enfreint le principe de responsabilité unique (SRP), sans se rendre compte qu’il avait déjà été violé.

Pour obtenir une solution SRP pure, commencez par déclencher l'événement, puis appelez tous les points d'ancrage pour cet événement, qui sont désormais au nombre de trois: enregistrer, consigner et enfin envoyer des courriels.

Soit vous déclenchez l’événement d’abord, soit vous devez ajouter quelque chose SaveChanges. Votre solution est un hybride entre les deux. Il ne traite pas de la violation existante, mais encourage à l'empêcher de se multiplier au-delà de trois choses. Refactoriser le code existant pour le rendre conforme à SRP pourrait nécessiter plus de travail que ce qui est strictement nécessaire. C'est à vous de décider jusqu'où ils veulent aller.

Le code violait déjà le SRP - la même classe était responsable de la communication avec le contexte de données et de la journalisation.

Vous venez de l'améliorer pour avoir 3 responsabilités.

Une façon de réduire les responsabilités à une responsabilité serait d’abstraire le _userRepository; faites-en un diffuseur de commande.

Il a un ensemble de commandes, plus un ensemble d'auditeurs. Il reçoit des commandes et les diffuse à ses auditeurs. Peut-être ces auditeurs sont-ils commandés et peuvent-ils même dire que la commande a échoué (à son tour, elle est diffusée aux auditeurs déjà avertis).

Maintenant, la plupart des commandes ne peuvent avoir qu'un seul auditeur (le contexte de données). SaveChanges, avant vos modifications, a 2 - le contexte de données, puis l'enregistreur.

Votre modification ajoute ensuite un autre écouteur pour enregistrer les modifications, qui consiste à générer de nouveaux événements créés par l'utilisateur dans le service d'événements.

Il y a quelques avantages à cela. Vous pouvez maintenant supprimer, mettre à niveau ou répliquer le code de journalisation sans vous soucier du reste de votre code. Vous pouvez ajouter plus de déclencheurs lors des modifications de sauvegarde pour plus de choses qui en ont besoin.

Tout cela est décidé lors de la _userRepositorycréation et de la connexion de l'outil (ou peut-être que ces fonctionnalités supplémentaires sont ajoutées / supprimées à la volée; la possibilité d'ajouter / d'améliorer la journalisation pendant l'exécution de l'application pourrait être utile).