Aplicabilidad del principio de responsabilidad única

40

Recientemente me topé con un problema arquitectónico aparentemente trivial. Tenía un repositorio simple en mi código que se llamaba así (el código está en C #):

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

SaveChanges fue un contenedor simple que confirma los cambios en la base de datos:

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

Luego, después de un tiempo, necesitaba implementar una nueva lógica que enviara notificaciones por correo electrónico cada vez que se creaba un usuario en el sistema. Como había muchas llamadas hacia _userRepository.Add()y SaveChangesalrededor del sistema, decidí actualizar de SaveChangesesta manera:

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

De esta forma, el código externo podría suscribirse a UserCreatedEvent y manejar la lógica de negocios necesaria que enviaría notificaciones.

Pero se me señaló que mi modificación de SaveChangesviolaba el principio de Responsabilidad Única, y eso SaveChangessolo debería salvar y no disparar ningún evento.

¿Es este un punto válido? Me parece que la generación de un evento aquí es esencialmente lo mismo que el registro: simplemente agregando algunas funciones secundarias a la función. Y SRP no le prohíbe usar eventos de inicio de sesión o disparo en sus funciones, solo dice que dicha lógica debe encapsularse en otras clases, y está bien que un repositorio llame a estas otras clases.

Andre Borges
fuente
22
Su réplica es: "OK, entonces ¿cómo se escribe de modo que no viola SRP pero todavía permite que un solo punto de modificación?"
Robert Harvey
43
Mi observación es que organizar un evento no agrega una responsabilidad adicional. De hecho, todo lo contrario: delega la responsabilidad en otro lugar.
Robert Harvey
Creo que su compañero de trabajo tiene razón, pero su pregunta es válida y útil, ¡así que votó!
Andrés F.
16
No existe una definición definitiva de responsabilidad única. La persona que señala que viola SRP es correcta usando su definición personal y usted es correcto usando su definición. Creo que su diseño está perfectamente bien con la advertencia de que este evento no es único, ya que otras funciones similares se realizan de diferentes maneras. La consistencia es mucho, mucho, mucho más importante a la que prestarle atención que alguna directriz vaga como SRP que llevada al extremo termina con toneladas de clases muy fáciles de entender que nadie sabe cómo hacer funcionar en un sistema.
Dunk

Respuestas:

14

Sí, puede ser un requisito válido tener un repositorio que active ciertos eventos en ciertas acciones como Addo SaveChanges, y no voy a cuestionar esto (como algunas otras respuestas) solo porque su ejemplo específico de agregar usuarios y enviar correos electrónicos puede parecer un poco artificial A continuación, supongamos que este requisito está perfectamente justificado en el contexto de su sistema.

Entonces , , codificar la mecánica del evento, así como el registro y el guardado en un método viola el SRP . Para muchos casos, es probablemente una violación aceptable, especialmente cuando nadie quiere distribuir las responsabilidades de mantenimiento de "guardar cambios" y "elevar evento" a diferentes equipos / mantenedores. Pero supongamos que un día alguien quiere hacer exactamente esto, ¿se puede resolver de una manera simple, quizás poniendo el código de esas preocupaciones en diferentes bibliotecas de clases?

La solución a esto es dejar que su repositorio original siga siendo responsable de enviar cambios a la base de datos, nada más, y hacer un repositorio proxy que tenga exactamente la misma interfaz pública, reutilice el repositorio original y agregue la mecánica de eventos adicional a los métodos.

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

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

Puede llamar a la clase proxy a, NotifyingRepositoryo ObservableRepositorysi lo desea, siguiendo las líneas de la respuesta altamente votada de @Pedter (que en realidad no dice cómo resolver la violación de SRP, solo dice que la violación está bien).

La clase de repositorio nueva y la antigua deben derivar de una interfaz común, como se muestra en la descripción clásica del patrón Proxy .

Luego, en su código original, inicialícelo _userRepositorypor un objeto de la nueva EventFiringUserRepoclase. De esa manera, mantienes el repositorio original separado de la mecánica del evento. Si es necesario, puede hacer que el evento active el repositorio y el repositorio original uno al lado del otro y deje que las personas que llaman decidan si usan el primero o el segundo.

Para abordar una inquietud mencionada en los comentarios: ¿eso no conduce a proxies sobre proxies sobre proxies, y así sucesivamente? En realidad, agregar la mecánica del evento crea una base para agregar requisitos adicionales del tipo "enviar correos electrónicos" simplemente suscribiéndose a los eventos, por lo que también se adhiere al SRP con esos requisitos, sin ningún proxy adicional. Pero lo único que debe agregarse una vez aquí es la mecánica del evento en sí.

Si este tipo de separación realmente vale la pena en el contexto de su sistema, es algo que usted y su revisor deben decidir por sí mismos. Probablemente no separaría el registro del código original, ni usando otro proxy ni agregando un registrador al evento de escucha, aunque sería posible.

Doc Brown
fuente
3
Además de esta respuesta. Hay alternativas a los proxies, como AOP .
Laiv
1
Creo que se pierde el punto, no es que generar un evento rompa el SRP, es que generar un evento solo para usuarios "nuevos" requiere que el repositorio sea responsable de saber qué constituye un usuario "nuevo" en lugar de un "recién agregado a mí". "usuario
Ewan
@Ewan: por favor lea la pregunta nuevamente. Se trataba de un lugar en el código que realiza ciertas acciones que deben combinarse con otras acciones fuera de la responsabilidad civil de ese objeto. Y poner la acción y el evento en un solo lugar fue cuestionado por un evaluador por romper el SRP. El ejemplo de "salvar nuevos usuarios" es solo para fines de demostración, llame al ejemplo inventado si lo desea, pero en mi humilde opinión no es el punto de la pregunta.
Doc Brown
2
Esta es la mejor / respuesta correcta de la OMI. No solo mantiene SRP, sino que también mantiene el principio Abierto / Cerrado. Y piense en todas las pruebas automatizadas que los cambios dentro de la clase podrían romper. La modificación de las pruebas existentes cuando se agrega una nueva funcionalidad es un gran olor.
Keith Payne
1
¿Cómo funciona esta solución si hay una transacción en curso? Cuando eso sucede, en SaveChanges()realidad no crea el registro de la base de datos, y podría terminar siendo revertido. Parece que tendría que anular AcceptAllChangeso suscribirse al evento TransactionCompleted.
John Wu
29

Enviar una notificación de que el almacén de datos persistente ha cambiado parece algo sensato al guardar.

Por supuesto, no debe tratar Agregar como un caso especial: también debería activar eventos para Modificar y Eliminar. Es el tratamiento especial del caso "Agregar" lo que huele, obliga al lector a explicar por qué huele, y finalmente lleva a algunos lectores del código a concluir que debe violar SRP.

Un repositorio de "notificación" que se puede consultar, cambiar y disparar eventos en los cambios, es un objeto perfectamente normal. Puede esperar encontrar múltiples variaciones de los mismos en casi cualquier proyecto de tamaño decente.


¿Pero es un repositorio de "notificaciones" lo que realmente necesita? Usted mencionó C #: Muchas personas estarían de acuerdo en que usar un System.Collections.ObjectModel.ObservableCollection<>lugar en lugar de System.Collections.Generic.List<>cuando lo último es todo lo que necesita es todo tipo de cosas malas e incorrectas, pero pocas señalarían de inmediato a SRP.

Lo que estás haciendo ahora es cambiar tu UserList _userRepositorypor un ObservableUserCollection _userRepository. Si ese es el mejor curso de acción o no, depende de la aplicación. Pero aunque sin duda hace que el _userRepositoryconsiderablemente menos liviano, en mi humilde opinión, no viola SRP.

Peter
fuente
El problema con el uso ObservableCollectionpara este caso es que desencadena el evento equivalente no en la llamada a SaveChanges, sino en la llamada a Add, lo que conduciría a un comportamiento muy diferente al que se muestra en el ejemplo. Vea mi respuesta sobre cómo mantener el repositorio original liviano y aún mantener el SRP manteniendo intacta la semántica.
Doc Brown
@DocBrown invoqué las clases conocidas ObservableCollection<>y List<>para comparación y contexto. No quise recomendar el uso de las clases reales ni para la implementación interna ni para la interfaz externa.
Peter
De acuerdo, pero incluso si el OP agregaría eventos a "Modificar" y "Eliminar" (lo que creo que el OP omitió para mantener la pregunta concisa, en aras de la simplicidad), creo que un revisor podría llegar fácilmente a la conclusión de una violación de SRP Probablemente sea aceptable, pero ninguno que no pueda resolverse si es necesario.
Doc Brown
16

Sí, es una violación del principio de responsabilidad única y un punto válido.

Un mejor diseño sería tener un proceso separado para recuperar 'nuevos usuarios' del repositorio y enviar los correos electrónicos. Realizar un seguimiento de qué usuarios han recibido un correo electrónico, fallas, reenvíos, etc., etc.

De esta forma, puede manejar errores, bloqueos y cosas similares, así como evitar que su repositorio tome todos los requisitos que tienen la idea de que los eventos suceden "cuando algo se compromete con la base de datos".

El repositorio no sabe que un usuario que agregue es un usuario nuevo. Su responsabilidad es simplemente almacenar al usuario.

Probablemente valga la pena ampliar los comentarios a continuación.

El repositorio no sabe que un usuario que agregue es un usuario nuevo; sí, tiene un método llamado Agregar. Su semántica implica que todos los usuarios agregados son usuarios nuevos. Combine todos los argumentos pasados ​​a Agregar antes de llamar a Guardar, y obtendrá todos los nuevos usuarios

Incorrecto. Estás combinando "Agregado al repositorio" y "Nuevo".

"Agregado al repositorio" significa exactamente lo que dice. Puedo agregar y eliminar y volver a agregar usuarios a varios repositorios.

"Nuevo" es el estado de un usuario definido por las reglas comerciales.

Actualmente, la regla de negocio podría ser "Nuevo == recién agregado al repositorio", pero eso no significa que no sea una responsabilidad separada conocer y aplicar esa regla.

Debe tener cuidado para evitar este tipo de pensamiento centrado en la base de datos. Tendrá procesos de casos extremos que agregarán usuarios no nuevos al repositorio y cuando les envíe correos electrónicos, toda la empresa dirá "¡Por supuesto que no son usuarios 'nuevos'! La regla real es X".

Esta respuesta es en mi humilde opinión: el repositorio es exactamente el único lugar central en el código que sabe cuándo se agregan nuevos usuarios

Incorrecto. Por las razones anteriores, además, no es una ubicación central a menos que realmente incluya el código de envío de correo electrónico en la clase en lugar de simplemente generar un evento.

Tendrá aplicaciones que usan la clase de repositorio, pero no tienen el código para enviar el correo electrónico. Cuando agrega usuarios en esas aplicaciones, no se enviará el correo electrónico.

Ewan
fuente
11
El repositorio no sabe que un usuario que agregue es un usuario nuevo ; sí, tiene un método llamado Add. Su semántica implica que todos los usuarios agregados son usuarios nuevos. Combine todos los argumentos pasados Addantes de llamar Save, y obtendrá todos los nuevos usuarios.
Andre Borges
Me gusta esta sugerencia Sin embargo, el pragmatismo prevalece sobre la pureza. Dependiendo de las circunstancias, agregar una capa arquitectónica completamente nueva a una aplicación existente puede ser difícil de justificar si todo lo que necesita hacer es literalmente enviar un solo correo electrónico cuando se agrega un usuario.
Alexander
Pero el evento no dice usuario agregado. Dice usuario creado. Si consideramos nombrar las cosas correctamente y estamos de acuerdo con las diferencias semánticas entre agregar y crear, entonces el evento en el fragmento tiene un nombre incorrecto o está mal ubicado. No creo que el crítico haya tenido nada en contra de notificar repositorios. Probablemente le preocupaba el tipo de evento y sus efectos secundarios.
Laiv
77
@Andre Nuevo en el repositorio, pero no necesariamente "nuevo" en el sentido comercial. Es la combinación de estas dos ideas lo que oculta la responsabilidad adicional a primera vista. Podría importar una tonelada de usuarios antiguos a mi nuevo repositorio, o eliminar y volver a agregar un usuario, etc. Habrá reglas comerciales sobre lo que un "nuevo usuario" está más allá de "se ha agregado al dB"
Ewan
1
Nota para el moderador: su respuesta no es una entrevista periodística. Si tiene ediciones, incorpórelas naturalmente en su respuesta sin crear todo el efecto de "noticias de última hora". No somos un foro de discusión.
Robert Harvey
7

¿Es este un punto válido?

Sí lo es, aunque depende mucho de la estructura de su código. No tengo el contexto completo, así que intentaré hablar en general.

Me parece que la generación de un evento aquí es esencialmente lo mismo que el registro: simplemente agregando algunas funciones secundarias a la función.

Absolutamente no lo es. El registro no es parte del flujo de negocios, se puede deshabilitar, no debe causar efectos secundarios (comerciales) y no debe influir en el estado y la salud de su aplicación de ninguna manera, incluso si por alguna razón no pudo iniciar sesión nada más. Ahora compara eso con la lógica que agregaste.

Y SRP no le prohíbe el uso de eventos de inicio de sesión o disparo en sus funciones, solo dice que dicha lógica debería encapsularse en otras clases, y está bien que un repositorio llame a estas otras clases.

SRP funciona en conjunto con ISP (S e I en SOLID). Terminas con muchas clases y métodos que hacen cosas muy específicas y nada más. Están muy enfocados, son muy fáciles de actualizar o reemplazar, y en general son más fáciles de probar. Por supuesto, en la práctica también tendrá algunas clases más grandes que se ocupan de la orquestación: tendrán una serie de dependencias y se centrarán no en acciones atomizadas, sino en acciones comerciales, que pueden requerir múltiples pasos. Siempre y cuando el contexto comercial sea claro, también se les puede llamar responsabilidad única, pero como usted dijo correctamente, a medida que el código crece, es posible que desee abstraerlo en nuevas clases / interfaces.

Ahora volvamos a su ejemplo particular. Si absolutamente debe enviar una notificación cada vez que se crea un usuario y tal vez incluso realizar otras acciones más especializadas, entonces podría crear un servicio separado que encapsule este requisito, algo así como UserCreationService, que expone un método Add(user), que maneja tanto el almacenamiento (la llamada a su repositorio) y la notificación como una sola acción comercial. O hazlo en tu fragmento original, después de_userRepository.SaveChanges();

asíncrono
fuente
2
El registro no es parte del flujo de negocios : ¿cómo es esto relevante en el contexto de SRP? Si el propósito de mi evento fuera enviar nuevos datos de usuario a Google Analytics, deshabilitarlo tendría el mismo efecto comercial que deshabilitar el registro: no es crítico, pero es bastante molesto. ¿Cuál es la regla general para agregar / no agregar nueva lógica a una función? "¿Deshabilitarlo causará importantes efectos secundarios comerciales?"
Andre Borges
2
If the purpose of my event would be to send new user data to Google Analytics - then disabling it would have the same business effect as disabling logging: not critical, but pretty upsetting . ¿Qué pasa si está disparando eventos prematuros que causan "noticias" falsas? ¿Qué sucede si la analítica toma en cuenta a los "usuarios" que finalmente no se crearon debido a errores con la transacción de base de datos? ¿Qué sucede si la empresa está tomando decisiones sobre premisas falsas, respaldadas por datos imprecisos? Estás demasiado concentrado en el aspecto técnico del problema. "A veces no puedes ver la madera de los árboles"
Laiv
@Laiv, estás haciendo un punto válido, pero este no es el punto de mi pregunta o esta respuesta. La pregunta es si esta es una solución válida en el contexto de SRP, así que supongamos que no hay errores de transacción DB.
Andre Borges
Básicamente me estás pidiendo que te diga lo que quieres escuchar. Solo te doy alcance. Un alcance más amplio para que usted decida si SRP importa o no porque SRP es inútil sin el contexto adecuado. En mi opinión, la forma en que aborda la preocupación es incorrecta porque solo se está centrando en la solución técnica. Debe dar suficiente relevancia a todo el contexto. Y sí, DB podría fallar. Existe la posibilidad de que ocurra y no debe omitir eso, porque, como sabe, las cosas suceden y estas cosas podrían cambiar de opinión con respecto a las dudas sobre SRP u otras buenas prácticas.
Laiv
1
Dicho esto, recuerde que los principios no son reglas escritas en piedra. Son permeables (adaptativos). Como puede ver, están abiertos a interpretación. Su crítico tiene una interpretación y usted tiene otra. Intente ver lo que ve, resuelva sus dudas e inquietudes, o deje que él / ella resuelva las suyas. No encontrará la respuesta "correcta" aquí. La respuesta correcta depende de usted y su revisor para encontrar, preguntando primero los requisitos (funcionales y no funcionales) del proyecto.
Laiv
4

SRP es, teóricamente, sobre personas , como explica el tío Bob en su artículo El principio de responsabilidad única . Gracias Robert Harvey por proporcionarlo en tu comentario.

La pregunta correcta es:

¿Qué "parte interesada" agregó el requisito de "enviar correos electrónicos"?

Si esa parte interesada también está a cargo de la persistencia de los datos (poco probable pero posible), entonces esto no viola el SRP. De lo contrario, lo hace.

user949300
fuente
2
Interesante: nunca escuché sobre esta interpretación del SRP. ¿Tiene alguna sugerencia para obtener más información / literatura sobre esta interpretación?
Sleske
2
@sleske: Del propio tío Bob : "Y esto llega al meollo del Principio de responsabilidad única. Este principio es sobre las personas. Cuando escribes un módulo de software, quieres asegurarte de que cuando se soliciten cambios, esos cambios solo puedan originarse "de una sola persona, o mejor dicho, un solo grupo de personas estrechamente acopladas que representan una única función empresarial estrechamente definida".
Robert Harvey
Gracias Robert OMI, el nombre "Principio de responsabilidad única" es terrible, ya que suena simple, pero muy pocas personas dan seguimiento a cuál es el significado de "responsabilidad". Algo así como cómo OOP ha mutado de muchos de sus conceptos originales, y ahora es un término bastante sin sentido.
user949300
1
Sí. Eso es lo que sucedió con el término REST. Incluso Roy Fielding dice que la gente lo está usando mal.
Robert Harvey
Aunque la cita está relacionada, creo que esta respuesta no considera que el requisito de "enviar correos electrónicos" no sea uno de los requisitos directos de los que se trata la pregunta de violación de SRP. Sin embargo, al decir "Qué" parte interesada "agregó el requisito de" evento de aumento " , esta respuesta estaría más relacionada con la pregunta real. Modifiqué mi propia respuesta un poco para aclarar esto.
Doc Brown
2

Si bien técnicamente no hay nada de malo en los repositorios que notifican eventos, sugeriría mirarlo desde un punto de vista funcional donde su conveniencia suscite algunas preocupaciones.

Crear un usuario, decidir qué es un nuevo usuario y su persistencia son 3 cosas diferentes .

Premisa mía

Tenga en cuenta la premisa anterior antes de decidir si el repositorio es el lugar adecuado para notificar eventos empresariales (independientemente del SRP). Tenga en cuenta que dije evento de negocios porque para mí UserCreatedtiene una connotación diferente de UserStoredoUserAdded 1 . También consideraría que cada uno de esos eventos está dirigido a diferentes audiencias.

Por un lado, la creación de usuarios es una regla específica del negocio que puede o no implicar persistencia. Puede involucrar más operaciones comerciales, involucrando más operaciones de base de datos / red. Operaciones que la capa de persistencia desconoce. La capa de persistencia no tiene suficiente contexto para decidir si el caso de uso finalizó con éxito o no.

Por otro lado, no es necesariamente cierto que _dataContext.SaveChanges();haya persistido al usuario con éxito. Dependerá de la duración de la transacción de la base de datos. Por ejemplo, podría ser cierto para bases de datos como MongoDB, cuyas transacciones son atómicas, pero no podría serlo, para RDBMS tradicionales que implementan transacciones ACID donde podría haber más transacciones involucradas y aún por comprometerse.

¿Es este un punto válido?

Podría ser. Sin embargo, me atrevería a decir que no es solo una cuestión de SRP (técnicamente hablando), sino también una cuestión de conveniencia (funcionalmente hablando).

  • ¿Es conveniente disparar eventos comerciales desde componentes que desconocen las operaciones comerciales en curso?
  • ¿Representan el lugar correcto tanto como el momento adecuado para hacerlo?
  • ¿Debo permitir que estos componentes orquenen mi lógica empresarial a través de notificaciones como esta?
  • ¿Podría invalidar los efectos secundarios causados ​​por eventos prematuros? 2

Me parece que la aparición de un evento aquí es esencialmente lo mismo que iniciar sesión

Absolutamente no. Sin embargo, el registro no debe tener efectos secundarios, ya que sugirió que UserCreatedes probable que el evento provoque otras operaciones comerciales. Como notificaciones. 3

solo dice que dicha lógica debería encapsularse en otras clases, y está bien que un repositorio llame a estas otras clases

No necesariamente cierto. SRP no es una preocupación específica de clase solamente. ¡Funciona en diferentes niveles de abstracciones, como capas, bibliotecas y sistemas! Se trata de la cohesión, de mantener juntos lo que cambia por las mismas razones de la mano de los mismos interesados . Si la creación del usuario ( caso de uso ) cambia, es probable que el momento y los motivos del evento también cambien.


1: Nombrar las cosas adecuadamente también es importante.

2: Digamos que enviamos UserCreateddespués _dataContext.SaveChanges();, pero la transacción completa de la base de datos falló más tarde debido a problemas de conexión o violaciones de restricciones. Tenga cuidado con la transmisión prematura de eventos, porque sus efectos secundarios pueden ser difíciles de deshacer (si eso es posible).

3: Los procesos de notificación que no se tratan adecuadamente pueden provocar que se activen notificaciones que no se pueden deshacer / sup>

Laiv
fuente
1
+1 Muy buen punto sobre la duración de la transacción. Puede ser prematuro afirmar que el usuario ha sido creado, porque pueden ocurrir retrocesos; y a diferencia de un registro, es probable que alguna otra parte de la aplicación haga algo con el evento.
Andrés F.
2
Exactamente. Los eventos denotan certeza. Algo sucedió pero se acabó.
Laiv
1
@Laiv: Excepto cuando no lo hacen. Microsoft tiene todo tipo de eventos con el prefijo Beforeo Previewque no garantiza en absoluto la certeza.
Robert Harvey
1
@ jpmc26: Sin una alternativa, su sugerencia no es útil.
Robert Harvey
1
@ jpmc26: Entonces, su respuesta es "cambiar a un ecosistema de desarrollo completamente diferente con un conjunto completamente diferente de herramientas y características de rendimiento". Llámame contrario, pero me imagino que esto no es factible para la gran mayoría de los esfuerzos de desarrollo.
Robert Harvey
1

No, esto no viola el SRP.

Muchos parecen pensar que el Principio de Responsabilidad Única significa que una función solo debe hacer "una cosa", y luego quedar atrapados en la discusión sobre lo que constituye "una cosa".

Pero eso no es lo que significa el principio. Se trata de preocupaciones a nivel empresarial. Una clase no debe implementar múltiples preocupaciones o requisitos que pueden cambiar de forma independiente a nivel comercial. Digamos que una clase almacena al usuario y envía un mensaje de bienvenida codificado por correo electrónico. Múltiple independiente preocupaciones podrían causar que los requisitos de dicha clase cambien. El diseñador podría requerir que la hoja de estilo / html del correo cambie. El experto en comunicaciones podría requerir que se modifique la redacción del correo. Y el experto en UX podría decidir que el correo debería enviarse en un punto diferente del flujo de incorporación. Por lo tanto, la clase está sujeta a múltiples cambios de requisitos de fuentes independientes. Esto viola el SRP.

Pero disparar un evento no viola el SRP, ya que el evento solo depende de salvar al usuario y no de ninguna otra preocupación. Los eventos son en realidad una buena manera de mantener el SRP, ya que puede guardar un correo electrónico activado por el guardado sin que el repositorio se vea afectado, o incluso saber, el correo.

JacquesB
fuente
1

No te preocupes por el principio de responsabilidad única. No va a ayudarlo a tomar una buena decisión aquí porque puede elegir subjetivamente un concepto particular como "responsabilidad". Podría decir que la responsabilidad de la clase es administrar la persistencia de datos en la base de datos, o podría decir que su responsabilidad es realizar todo el trabajo relacionado con la creación de un usuario. Estos son solo niveles diferentes del comportamiento de la aplicación, y ambos son expresiones conceptuales válidas de una "responsabilidad única". Por lo tanto, este principio no es útil para resolver su problema.

El principio más útil para aplicar en este caso es el principio de menor sorpresa . Entonces, hagamos la pregunta: ¿es sorprendente que un repositorio con la función principal de datos persistentes en una base de datos también envíe correos electrónicos?

Sí, es muy sorprendente. Estos son dos sistemas externos completamente separados, y el nombre SaveChangesno implica también enviar notificaciones. El hecho de que delegue esto en un evento hace que el comportamiento sea aún más sorprendente, ya que alguien que lee el código ya no puede ver fácilmente qué comportamientos adicionales se invocan. La indirección perjudica la legibilidad. A veces, los beneficios valen los costos de legibilidad, pero no cuando se invoca automáticamente un sistema externo adicional que tiene efectos observables para los usuarios finales. (El registro puede excluirse aquí ya que su efecto es esencialmente el mantenimiento de registros con fines de depuración. Los usuarios finales no consumen el registro, por lo que no hay ningún daño en el registro siempre.) Aún peor, esto reduce la flexibilidad en el tiempo de enviar el correo electrónico, lo que hace imposible intercalar otras operaciones entre guardar y la notificación.

Si su código generalmente necesita enviar una notificación cuando un usuario se crea con éxito, puede crear un método que lo haga:

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

Pero si esto agrega valor depende de los detalles de su aplicación.


De hecho, desalentaría la existencia del SaveChangesmétodo. Este método presumiblemente confirmará una transacción de la base de datos, pero otros repositorios podrían haber modificado la base de datos en la misma transacción . El hecho de que los compromete a todos nuevamente es sorprendente, ya que SaveChangesestá específicamente relacionado con esta instancia del repositorio de usuarios.

El patrón más directo para administrar una transacción de base de datos es un usingbloque externo :

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

Esto le da al programador un control explícito sobre cuándo se guardan los cambios para todos los repositorios, obliga al código a documentar explícitamente la secuencia de eventos que deben ocurrir antes de una confirmación, asegura que se emita una reversión por error (suponiendo que se DataContext.Disposeemita una reversión) y evita que se oculte conexiones entre clases con estado.

También preferiría no enviar el correo electrónico directamente en la solicitud. Sería más robusto registrar la necesidad de una notificación en una cola. Esto permitiría un mejor manejo de fallas. En particular, si se produce un error al enviar el correo electrónico, se puede volver a intentar más tarde sin interrumpir el guardado del usuario, y se evita el caso en el que se crea el usuario pero el sitio devuelve un error.

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

Es mejor confirmar primero la cola de notificaciones, ya que el consumidor de la cola puede verificar que el usuario existe antes de enviar el correo electrónico, en caso de que context.SaveChanges()falle la llamada. (De lo contrario, necesitará una estrategia de compromiso de dos fases completa para evitar errores de seguridad).


El resultado final es ser práctico. Realmente piense en las consecuencias (tanto en términos de riesgo como de beneficio) de escribir código de una manera particular. Me parece que el "principio de responsabilidad única" no me ayuda mucho a hacerlo, mientras que el "principio de menor sorpresa" a menudo me ayuda a meterme en la cabeza de otro desarrollador (por así decirlo) y pensar en lo que podría suceder.

jpmc26
fuente
44
¿Es sorprendente que un repositorio con la función principal de datos persistentes en una base de datos también envíe correos electrónicos ? Creo que se perdió el punto de mi pregunta. Mi repositorio no está enviando correos electrónicos. Simplemente genera un evento, y cómo se maneja este evento es responsabilidad del código externo.
Andre Borges
44
Básicamente estás argumentando "no uses eventos".
Robert Harvey
3
[encogimiento de hombros] Los eventos son fundamentales para la mayoría de los marcos de UI. Elimina eventos, y esos marcos no funcionan en absoluto.
Robert Harvey
2
@ jpmc26: se llama ASP.NET Webforms. Apesta.
Robert Harvey
2
My repository is not sending emails. It just raises an eventcausa efecto. El repositorio está activando el proceso de notificación.
Laiv
0

Actualmente SaveChangeshace dos cosas: guarda los cambios y registra que lo hace. Ahora desea agregarle otra cosa: enviar notificaciones por correo electrónico.

Tuviste la inteligente idea de agregarle un evento, pero esto fue criticado por violar el Principio de Responsabilidad Única (SRP), sin notar que ya se había violado.

Para obtener una solución SRP pura, primero active el evento, luego llame a todos los ganchos para ese evento, de los cuales ahora hay tres: guardar, iniciar sesión y finalmente enviar correos electrónicos.

O disparas el evento primero o tienes que agregarlo SaveChanges. Su solución es un híbrido entre los dos. No aborda la violación existente, mientras que alienta a evitar que aumente más allá de tres cosas. Refactorizar el código existente para cumplir con SRP puede requerir más trabajo del estrictamente necesario. Depende de su proyecto qué tan lejos quieren llevar SRP.

CJ Dennis
fuente
-1

El código ya violaba el SRP: la misma clase era responsable de comunicarse con el contexto de datos y el registro.

Simplemente actualícelo a 3 responsabilidades.

Una forma de despojar las cosas a una responsabilidad sería abstraer el _userRepository; conviértalo en un emisor de comandos.

Tiene un conjunto de comandos, más un conjunto de oyentes. Obtiene comandos y los transmite a sus oyentes. Posiblemente esos oyentes están ordenados, y tal vez incluso puedan decir que el comando falló (que a su vez se transmite a los oyentes que ya habían sido notificados).

Ahora, la mayoría de los comandos pueden tener solo 1 oyente (el contexto de datos). SaveChanges, antes de sus cambios, tiene 2: el contexto de datos y luego el registrador.

Su cambio luego agrega otro oyente para guardar los cambios, que es generar nuevos eventos creados por el usuario en el servicio de eventos.

Hay algunos beneficios para esto. Ahora puede eliminar, actualizar o replicar el código de registro sin que le importe el resto del código. Puede agregar más desencadenantes en los cambios guardados para más cosas que lo necesitan.

Todo esto se decide cuando _userRepositoryse crea y se conecta (o, tal vez, esas características adicionales se agregan / eliminan sobre la marcha; poder agregar / mejorar el registro mientras se ejecuta la aplicación podría ser útil).

Yakk
fuente