Recientemente tuve un debate con otro desarrollador sobre la siguiente clase:
public class GroupBillingPayment
{
public void Save(IGroupBillingPayment model)
{
if (model == null || UserInfo.UserID == 0)
{
throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
}
Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
Mapper.Map(model, groupBillingPayment);
ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
groupBillingPayment.UpdatedBy = UserInfo.UserID;
groupBillingPayment.UpdatedOn = DateTime.Now;
RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
UpdateGroupBilling([Parameters])
}
}
Creo UpdateGroupBilling
que no debería llamarse dentro del método de guardar, ya que viola el principio de responsabilidad única. Pero, dice que cada vez que se realiza un pago, la facturación debe actualizarse. Por lo tanto, este es el enfoque correcto.
Mi pregunta, ¿ se está violando SRP aquí? En caso afirmativo, ¿cómo podemos refactorizarlo mejor para que se cumplan nuestros dos criterios?
Respuestas:
Lo miraría de esta manera:
Un método debe llamar a otros métodos (en el mismo u otros objetos) que lo convierte en un método compuesto o hacer "cálculos primitivos" (mismo nivel de abstracción).
La responsabilidad de un método compuesto es "llamar a otros métodos".
Entonces, si su
Save
método hace algunos "cálculos primitivos" (por ejemplo, verifica los valores de retorno), se podría violar el SPR. Si este "cálculo primitivo" vive en otro método llamado por suSave
método, no se viola SRP.La versión actualizada del
Save
método no sigue el principio de la capa de abstracción única . Como este es el problema más importante, debe extraerlo a un nuevo método.Esto se convertirá
Save
en un método compuesto . Tal como está escrito, la responsabilidad de un método compuesto es "llamar a otros métodos". Por lo tanto, llamarUpdateGroupBilling([Parameters])
aquí no es una violación del SRP, sino una decisión de un caso comercial.fuente
Save
El método ha sido actualizado. Por favor, vea si esto ayudaLa responsabilidad individual puede interpretarse como una función / clase que debe tener solo una razón para cambiar.
Por lo tanto, su
Save
método actual violará ese principio, porque debería cambiarse por más de un motivo.1. Guardar la lógica modificada
2. Si decide actualizar otra cosa además de actualizar el grupo de facturación
Puede refactorizarlo introduciendo un
SaveModel
método que solo será responsable de guardar. Y presentamos otro método que combina todas las operaciones necesarias según sus requisitos. Entonces terminas con dos métodosDonde el
SaveModel
método tendrá la responsabilidad de guardar el modelo en la base de datos y una razón para cambiar, si la "lógica" de guardado cambiará.Save
El método ahora tiene la responsabilidad de llamar a todos los métodos requeridos para el proceso completo de "guardar", y tiene una razón para cambiar, si la cantidad del método requerido cambia.Creo que la validación también se puede mover fuera de
SaveModel
.fuente
La responsabilidad única es, en mi humilde opinión, un concepto no fácil de concretar.
Una regla general simple es:
Por un lado, es solo un indicador y, por otro lado, funciona mejor al revés: si están sucediendo dos cosas, no puede evitar usar las palabras »y« y, dado que está haciendo dos cosas, viola el SRP .
Pero, ¿significa eso, por otro lado, si haces más de una cosa, estás violando SRP ? No. Porque de lo contrario estabas limitado a código trivial y problemas triviales para resolver. Te lastimarías si fueras demasiado estricto en la interpretación.
Otra perspectiva sobre SRP es: un nivel de abstracción . Mientras estés lidiando con un nivel de abstracción, en general estás bien.
¿Qué significa todo eso para su pregunta?
Para decidir si se trata de una violación de SRP , es necesario saber qué está sucediendo en el
save()
Método. Si el método es, como su nombre, sugiere responsable de persistir el modelo en la base de datos, una llamada aUpdateGroupBilling
es en mi humilde opinión una violación de SRP , ya que está ampliando el contexto de "guardar un pago". Lo parafrasearía con "Estoy guardando el pago y actualizo la facturación grupal", que es, como dije antes, un (posible) indicador de violación de SRP .Por otro lado, si el método describe una "receta de pago", es decir, qué pasos hay que seguir en el proceso, y decide: cada vez que se completa un pago, debe guardarse (tratarse en otro lugar) y luego las facturas grupales deben actualizarse (tratarse en otro lugar), eso no sería una violación de SRP, ya que no está dejando la abstracción de la "receta": usted distingue entre qué hacer (en la receta) y dónde está hecho (en la clase / método / módulo correspondiente). Pero si eso es lo que hace su
save()
método (que describe los pasos a seguir), debe cambiarle el nombre.Sin más contexto, es difícil decir algo concreto.
Editar: actualizó su publicación inicial. Yo diría que este método viola el SRP y debería ser refactorizado. La recuperación de los datos debe ser factorizada (debe ser un parámetro de este método). La adición de datos (updateBy / On) debe hacerse en otro lugar. El ahorro debe hacerse en otro lugar. Entonces estaría bien irse
UpdateGroupBilling([Parameters])
como está.fuente
Es difícil estar seguro sin un contexto completo, pero creo que su intuición es correcta. Mi indicador SRP favorito personal es si vas a saber exactamente a dónde ir y cambiar algo para modificar una función meses después.
Una clase que define un tipo de pago no es donde esperaría ir para redefinir quién paga o cómo se realiza el pago. Esperaría que fuera uno de los muchos tipos de pago que simplemente brindan detalles críticos que otra parte de la aplicación utiliza para iniciar, realizar o revertir / cancelar una transacción, reuniendo esos detalles a través de una interfaz uniforme.
También parece una receta para una violación más general del principio DRY si va a tener varios tipos, cada uno de los cuales resolverá sus propias transacciones utilizando muchos de los mismos métodos. SOLID no siempre se siente 100% relevante para el desarrollador de JavaScript principalmente (aunque sospecho que mucho se explica mal), pero DRY es el estándar de oro en la metodología de programación general IMO. Cada vez que me encuentro con una idea que parece una extensión de DRY, lo considero. SRP es uno de esos. Si todos permanecen en el tema, es menos probable que sienta la tentación de repetirlo.
fuente
Si conceptualmente solo hay una forma de hacerlo
UpdateGroupBilling(...)
, y solo sucede en ese lugar, entonces probablemente esté bien donde está. Pero la pregunta está relacionada con el concepto, no con las circunstancias temporales, es decir, "por el momento solo lo hacemos aquí".Si no es el caso, entonces una forma de refactorizar sería utilizar Publicar / Suscribir y notificar cada vez que se guarda el pago, y hacer que la facturación se suscriba a ese evento y actualice las entradas relevantes. Esto es bueno, especialmente si necesita tener varios tipos de facturación que deben actualizarse. Otro método sería pensar en la facturación como un patrón de estrategia, es decir, elegir uno y usarlo, o aceptarlo como un parámetro. O, si la facturación no siempre ocurre, puede agregarla como decorador, etc., etc. Pero, de nuevo, esto depende de su modelo conceptual y de cómo quiera pensarlo, por lo que tendrá que resolverlo. .
Sin embargo, una cosa importante a considerar es el manejo de errores. Si la facturación falla, ¿qué sucede con las operaciones anteriores?
fuente
Creo que este diseño está violando el SRP, pero es realmente fácil de arreglar y aún se necesita hacer todo lo demás.
Piénselo en los mensajes y en lo que sucede en este método. Debería estar guardando el
GroupBillingPayment
, pero no hay nada de malo siGroupBillingPayment
notifica a las clases que se ha guardado. Especialmente si está implementando un patrón que expone explícitamente ese comportamiento.Podrías usar el patrón Observador .
Aquí hay un ejemplo de cómo funcionaría en su caso:
Ahora todo lo que tiene que hacer es crear uno o más
Observers
que estarán vinculadosGroupBillingPayment
y, por lo tanto, recibir una notificación cada vez que se guarde. Mantiene sus propias dependencias, no sabe qué notificó, por lo que no depende de ellas en absoluto.fuente
Desea lograr X. A menos que X sea bastante trivial, escriba un método que haga todo lo posible para lograr X, llame al método "generateX" y llame a ese método. La única responsabilidad de "AchieveX" es hacer todo lo posible para lograr X. "AchieveX" no debe hacer otras cosas.
Si X es complejo, es posible que se necesiten muchas acciones para lograr X. El conjunto de acciones necesarias puede cambiar con el tiempo, por lo que cuando eso suceda, cambiará el método AchieveX, pero si todo va bien, aún puede llamarlo.
En su ejemplo, la responsabilidad del método "Guardar" no es guardar la factura y actualizar la facturación grupal (dos responsabilidades), su responsabilidad es tomar todas las acciones necesarias para que la factura se registre permanentemente. Una responsabilidad Tal vez lo llamaste mal.
fuente
Si entiendo su punto, tendría el pago para realizar un evento como "Pago realizado" y la clase que maneja la facturación que lo suscribe. De esta manera, el manejador de pagos no sabe nada sobre la facturación.
fuente