De esta manera, estoy escribiendo este código es comprobable, pero ¿hay algo mal que me falta?

13

Tengo una interfaz llamada IContext. A los efectos de esto, realmente no importa lo que hace, excepto lo siguiente:

T GetService<T>();

Lo que hace este método es mirar el contenedor DI actual de la aplicación e intenta resolver la dependencia. Bastante estándar, creo.

En mi aplicación ASP.NET MVC, mi constructor se ve así.

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetService<ISomeService>();
    AnotherService = ctx.GetService<IAnotherService>();
}

Entonces, en lugar de agregar múltiples parámetros en el constructor para cada servicio (porque esto será realmente molesto y requerirá mucho tiempo para los desarrolladores que extienden la aplicación) Estoy usando este método para obtener servicios.

Ahora, se siente mal . Sin embargo, la forma en que lo estoy justificando actualmente es esto: puedo burlarme de él .

Yo puedo. No sería difícil burlarse IContextpara probar el controlador. Tendría que de todos modos:

public class MyMockContext : IContext
{
    public T GetService<T>()
    {
        if (typeof(T) == typeof(ISomeService))
        {
            // return another mock, or concrete etc etc
        }

        // etc etc
    }
}

Pero como dije, se siente mal. Cualquier pensamiento / abuso bienvenido.

Piscinas de hígado Número 9
fuente
8
Esto se llama Localizador de servicios y no me gusta. Se ha escrito mucho sobre el tema; consulte martinfowler.com/articles/injection.html y blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern para obtener información más detallada .
Benjamin Hodgson el
Del artículo de Martin Fowler: "A menudo escuché la queja de que este tipo de localizadores de servicios son malos porque no son verificables porque no puedes sustituirlos por implementaciones. Ciertamente puedes diseñarlos mal para entrar en esto tipo de problema, pero no tiene que hacerlo. En este caso, la instancia del localizador de servicios es solo un simple titular de datos. Puedo crear fácilmente el localizador con implementaciones de prueba de mis servicios ". ¿Podrías explicar por qué no te gusta? Tal vez en una respuesta?
LiverpoolsNumber9
8
Tiene razón, este es un mal diseño. Es muy fácil: public SomeClass(Context c). Este código es bastante claro, ¿no? Dice, that SomeClassdepende de a Context. Err, pero espera, no lo hace! Solo se basa en la dependencia Xque obtiene de Context. Eso significa que, cada vez que se realice un cambio en Contextél podría romper SomeObject, a pesar de que sólo cambió Contexts Y. Pero sí, sabes que solo Yno cambiaste X, así que SomeClassestá bien. Pero escribir un buen código no se trata de lo que usted sabe, sino de lo que sabe el nuevo empleado cuando mira su código la primera vez.
valenterry
@DocBrown Para mí eso es exactamente lo que dije: no veo la diferencia aquí. ¿Puedes por favor explicar más?
valenterry
1
@DocBrown Veo tu punto ahora. Sí, si su contexto es solo un paquete de todas las dependencias, entonces este no es un mal diseño. Sin embargo, puede ser un mal nombre, pero eso también es solo una suposición. OP debería aclarar si hay más métodos (objetos internos) del contexto. Además, discutir el código está bien, pero esto es programmers.stackexchange, así que para mí también deberíamos tratar de ver "detrás" de las cosas para mejorar el OP.
valenterry

Respuestas:

5

Tener uno en lugar de muchos parámetros en el constructor no es la parte problemática de este diseño . Mientras su IContextclase no sea más que una fachada de servicio , específicamente para proporcionar las dependencias utilizadas MyControllerBase, y no un localizador de servicio general utilizado en todo su código, esa parte de su código está en mi humilde opinión.

Su primer ejemplo podría cambiarse a

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetSomeService();
    AnotherService = ctx.GetAnotherService();
}

eso no sería un cambio sustancial de diseño de MyControllerBase. Si este diseño es bueno o malo depende solo del hecho si quieres

  • asegúrese TheContext, SomeServicey AnotherServicesiempre están todos inicializados con objetos simulados, o todos ellos con objetos reales
  • o, para permitir su inicialización con diferentes combinaciones de los 3 objetos (lo que significa, para este caso, necesitaría pasar los parámetros individualmente)

Por lo tanto, usar solo un parámetro en lugar de 3 en el constructor puede ser completamente razonable.

Lo que es problemático es IContextexponer el GetServicemétodo en público. En mi humilde opinión, debe evitar esto, en su lugar, mantenga los "métodos de fábrica" ​​explícitos. Entonces, ¿estará bien implementar los métodos GetSomeServicey GetAnotherServicede mi ejemplo utilizando un localizador de servicios? En mi humilde opinión eso depende. Mientras la IContextclase siga siendo una simple fábrica abstracta para el propósito específico de proporcionar una lista explícita de objetos de servicio, eso es aceptable en mi humilde opinión. Las fábricas abstractas suelen ser solo un código de "pegamento", que no tiene que ser probado por sí mismo. Sin embargo, debe preguntarse si en el contexto de métodos como GetSomeService, si realmente necesita un localizador de servicios, o si una llamada explícita de constructor no sería más simple.

Así que tenga cuidado, cuando se adhiere a un diseño donde la IContextimplementación es solo una envoltura alrededor de un GetServicemétodo público genérico , que permite resolver cualquier dependencia arbitraria por clases arbitrarias, entonces todo se aplica a lo que @BenjaminHodgson escribió en su respuesta.

Doc Brown
fuente
Estoy de acuerdo con ésto. El problema con el ejemplo es el GetServicemétodo genérico . Refactorizar a métodos explícitamente nombrados y escritos es mejor. Mejor aún sería explicar IContextexplícitamente las dependencias de la implementación en el constructor.
Benjamin Hodgson
1
@BenjaminHodgson: a veces puede ser mejor, pero no siempre es mejor. Usted nota el olor del código cuando la lista de parámetros de ctor se hace más y más larga. Vea mi respuesta anterior aquí: programmers.stackexchange.com/questions/190120/…
Doc Brown
@DocBrown el "olor a código" de la sobreinyección del constructor es indicativo de una violación del SRP, que es el verdadero problema. Simplemente envolver varios servicios en una clase Fachada, solo para exponerlos como propiedades, no hace nada para abordar el origen del problema. Por lo tanto, una Fachada no debería ser una simple envoltura alrededor de otros componentes, sino que idealmente debería ofrecer una API simplificada para consumirlos (o debería simplificarlos de alguna otra manera) ...
AlexFoxGill
... Recuerde que un "olor a código" como la sobreinyección del constructor no es un problema en sí mismo. Es solo una pista de que hay un problema más profundo en el código que generalmente se puede resolver refactorizando de manera sensata una vez que se ha identificado el problema
AlexFoxGill
¿Dónde estabas cuando Microsoft introdujo esto IValidatableObject?
RubberDuck
15

Este diseño se conoce como Service Locator * y no me gusta. Hay muchos argumentos en contra:

El Localizador de servicios lo acopla a su contenedor . Usando la inyección de dependencia regular (donde el constructor explica explícitamente las dependencias) puede reemplazar directamente su contenedor por uno diferente, o volver a new-expresiones. Con tu IContexteso no es realmente posible.

Service Locator oculta dependencias . Como cliente, es muy difícil saber qué necesita para construir una instancia de una clase. Necesita algún tipo de IContext, pero también necesita configurar el contexto para devolver los objetos correctos para hacer el MyControllerBasetrabajo. Esto no es del todo obvio por la firma del constructor. Con DI regular, el compilador le dice exactamente lo que necesita. Si su clase tiene muchas dependencias, debe sentir ese dolor porque lo estimulará a refactorizar. Service Locator oculta los problemas con los malos diseños.

Service Locator provoca errores en tiempo de ejecución . Si llama GetServicecon un parámetro de tipo incorrecto, obtendrá una excepción. En otras palabras, su GetServicefunción no es una función total. (Las funciones totales son una idea del mundo FP, pero básicamente significa que las funciones siempre deben devolver un valor). Es mejor dejar que el compilador ayude y le diga cuándo tiene las dependencias equivocadas.

El Localizador de servicios viola el Principio de sustitución de Liskov . ¡Debido a que su comportamiento varía según el argumento de tipo, Service Locator puede verse como si efectivamente tuviera un número infinito de métodos en la interfaz! Este argumento se explica en detalle aquí .

Service Locator es difícil de probar . Has dado un ejemplo de una IContextprueba falsa , lo cual está bien, pero seguramente es mejor no tener que escribir ese código en primer lugar. Simplemente inyecte sus dependencias falsas directamente, sin pasar por su localizador de servicios.

En resumen, simplemente no lo hagas . Parece una solución seductora para el problema de las clases con muchas dependencias, pero a la larga vas a hacerte la vida imposible.

* Estoy definiendo Service Locator como un objeto con un Resolve<T>método genérico que es capaz de resolver dependencias arbitrarias y se usa en toda la base de código (no solo en la raíz de composición). Esto no es lo mismo que Service Facade (un objeto que agrupa un pequeño conjunto conocido de dependencias) o Abstract Factory (un objeto que crea instancias de un solo tipo; el tipo de Abstract Factory puede ser genérico pero el método no lo es) .

Benjamin Hodgson
fuente
1
Está compilando sobre el patrón del Localizador de servicios (que estoy de acuerdo). Pero en realidad, en el ejemplo del OP, MyControllerBaseno está acoplado a un contenedor DI específico, ni es "realmente" un ejemplo para el antipatrón del Localizador de servicios.
Doc Brown
@DocBrown Estoy de acuerdo. No porque me facilite la vida, sino porque la mayoría de los ejemplos anteriores no son relevantes para mi código.
LiverpoolsNumber9
2
Para mí, el sello distintivo del antipatrón del Localizador de servicios es el GetService<T>método genérico . Resolver dependencias arbitrarias es el verdadero olor, que está presente y es correcto en el ejemplo del OP.
Benjamin Hodgson
1
Otro problema con el uso de un Localizador de servicios es que reduce la flexibilidad: solo puede tener una implementación de cada interfaz de servicio. Si construye dos clases que dependen de un IFrobnicator, pero luego decide que una debe usar su implementación original de DefaultFrobnicator, pero la otra realmente debería estar usando un decorador CacheingFrobnicator a su alrededor, debe cambiar el código existente, mientras que si está inyectando dependencias directamente, todo lo que tiene que hacer es cambiar su código de configuración (o archivo de configuración si está utilizando un marco DI). Por lo tanto, esta es una violación de OCP.
Julio
1
@DocBrown El GetService<T>()método permite que se soliciten clases arbitrarias: "Lo que hace este método es mirar el contenedor DI actual de la aplicación e intenta resolver la dependencia. Creo que es bastante estándar". . Estaba respondiendo a tu comentario en la parte superior de esta respuesta. Este es 100% un localizador de servicios
AlexFoxGill
5

Mark Seemann afirma claramente los mejores argumentos contra el antipatrón del Localizador de servicios, por lo que no profundizaré demasiado en por qué es una mala idea: es un viaje de aprendizaje que debe tomarse el tiempo para comprender por sí mismo (I También recomiendo el libro de Mark ).

OK, así que para responder la pregunta, volvamos a exponer su problema real :

Entonces, en lugar de agregar múltiples parámetros en el constructor para cada servicio (porque esto será realmente molesto y requerirá mucho tiempo para los desarrolladores que extienden la aplicación) Estoy usando este método para obtener servicios.

Hay una pregunta que aborda este problema en StackOverflow . Como se menciona en uno de los comentarios allí:

La mejor observación: "Uno de los maravillosos beneficios de la inyección de constructor es que hace que las violaciones del principio de responsabilidad única sean evidentemente evidentes".

Está buscando la solución a su problema en el lugar equivocado. Es importante saber cuándo una clase está haciendo demasiado. En su caso , sospecho que no hay necesidad de un "Controlador base". De hecho, en OOP casi nunca hay necesidad de herencia en absoluto . Las variaciones en el comportamiento y la funcionalidad compartida se pueden lograr por completo mediante el uso apropiado de interfaces, lo que generalmente da como resultado un código mejor factorizado y encapsulado, y sin necesidad de pasar dependencias a los constructores de superclase.

En todos los proyectos en los que he trabajado donde hay un controlador base, se realizó con el único fin de compartir propiedades y métodos convenientes, como IsUserLoggedIn()y GetCurrentUserId(). PARADA . Este es un mal uso horrible de la herencia. En su lugar, cree un componente que exponga estos métodos y dependa de él donde lo necesite. De esta manera, sus componentes seguirán siendo comprobables y sus dependencias serán obvias.

Aparte de cualquier otra cosa, al usar el patrón MVC siempre recomendaría controladores flacos . Puede leer más sobre esto aquí, pero la esencia del patrón es simple, que los controladores en MVC deberían hacer una sola cosa: manejar los argumentos pasados ​​por el marco MVC, delegando otras preocupaciones a algún otro componente. De nuevo, este es el Principio de responsabilidad única en el trabajo.

Realmente sería útil conocer su caso de uso para hacer un juicio más preciso, pero sinceramente, no puedo pensar en ningún escenario en el que una clase base sea preferible a las dependencias bien factorizadas.

AlexFoxGill
fuente
+1 - esta es una nueva versión de la pregunta que ninguna de las otras respuestas ha abordado realmente
Benjamin Hodgson
1
"Uno de los maravillosos beneficios de la inyección de constructor es que hace que las violaciones del principio de responsabilidad única sean evidentemente obvias" Me encanta eso. Muy buena respuesta. No estés de acuerdo con todo eso. Mi caso de uso es, curiosamente, no tener que duplicar el código en un sistema que tendrá más de 100 controladores (al menos). Sin embargo, re SRP: cada servicio inyectado tiene una responsabilidad única, al igual que el controlador que los usa a todos, ¿cómo podría un refractor eso?
LiverpoolsNumber9
1
@ LiverpoolsNumber9 La clave es mover la funcionalidad de BaseController a dependencias, hasta que no quede nada en BaseController, excepto protectedlas delegaciones de una línea a los nuevos componentes. Entonces puede perder BaseController y reemplazar esos protectedmétodos con llamadas directas a dependencias: esto simplificará su modelo y hará explícitas todas sus dependencias (¡lo cual es algo muy bueno en un proyecto con cientos de controladores!)
AlexFoxGill
@ LiverpoolsNumber9 - si pudieras copiar una porción de tu BaseController en un pastebin, podría hacer algunas sugerencias concretas
AlexFoxGill
-2

Estoy agregando una respuesta a esto basada en las contribuciones de todos los demás. Muchas gracias a todos. Primero, aquí está mi respuesta: "No, no hay nada de malo".

Respuesta de "Fachada de servicio" de Doc Brown. Acepté esta respuesta porque lo que estaba buscando (si la respuesta era "no") eran algunos ejemplos o alguna expansión sobre lo que estaba haciendo. Proporcionó esto al sugerir que A) tiene un nombre y B) probablemente hay mejores formas de hacerlo.

Respuesta del "Localizador de servicios" de Benjamin Hodgson Aunque aprecio el conocimiento que he adquirido aquí, lo que tengo no es un "Localizador de servicios". Es una "Fachada de servicio". Todo en esta respuesta es correcto pero no para mis circunstancias.

Respuestas de la USR

Abordaré esto con más detalle:

Estás renunciando a mucha información estática de esta manera. Está aplazando las decisiones de tiempo de ejecución como lo hacen muchos lenguajes dinámicos. De ese modo, pierde la verificación estática (seguridad), la documentación y el soporte de herramientas (autocompletar, refactorizaciones, encontrar usos, flujo de datos).

No pierdo ninguna herramienta y no pierdo ninguna escritura "estática". La fachada de servicio devolverá lo que configuré en mi contenedor DI, o default(T). Y lo que devuelve es "escrito". El reflejo está encapsulado.

No veo por qué agregar servicios adicionales como argumentos de constructor es una gran carga.

Ciertamente no es "raro". Como estoy usando un controlador base , cada vez que necesito cambiar su constructor, es posible que tenga que cambiar 10, 100, 1000 otros controladores.

Si usa un marco de inyección de dependencia, ni siquiera tendrá que pasar manualmente los valores de los parámetros. Por otra parte, pierde algunas ventajas estáticas, pero no tantas.

Estoy usando inyección de dependencia. Ese es el punto.

Y finalmente, el comentario de Jules sobre la respuesta de Benjamin de que no estoy perdiendo flexibilidad. Es mi fachada de servicio. Puedo agregar tantos parámetros GetService<T>como desee para distinguir entre diferentes implementaciones, tal como lo haría uno al configurar un contenedor DI. Entonces, por ejemplo, podría cambiar GetService<T>()para GetService<T>(string extraInfo = null)evitar este "problema potencial".


De todos modos, gracias de nuevo a todos. Ha sido realmente útil. Salud.

Piscinas de hígado Número 9
fuente
44
No estoy de acuerdo con que tenga una Fachada de servicio aquí. El GetService<T>()método puede (intentar) resolver dependencias arbitrarias . Esto lo convierte en un Localizador de servicios, no en una Fachada de servicios, como expliqué en la nota de pie de página de mi respuesta. Si tuviera que reemplazarlo con un pequeño conjunto de GetServiceX()/ GetServiceY()métodos, como sugiere @DocBrown, entonces sería una Fachada.
Benjamin Hodgson
2
Debe leer y prestar mucha atención a la última sección de este artículo sobre el Localizador de servicios abstractos , que es esencialmente lo que está haciendo. He visto que este antipatrón corrompe un proyecto completo: preste especial atención a la cita "empeorará su vida como desarrollador de mantenimiento porque necesitará usar cantidades considerables de poder mental para comprender las implicaciones de cada cambio que realice "
AlexFoxGill
3
Sobre escritura estática: te estás perdiendo el punto. Si intento escribir código que no proporciona una clase usando DI con un parámetro de constructor que necesita, no se compilará. Eso es seguridad estática en tiempo de compilación contra problemas. Si escribe su código, proporcionando a su constructor un IContext que no se ha configurado correctamente para proporcionar el argumento que realmente necesita, entonces se compilará y fallará en tiempo de ejecución
Ben Aaronson
3
@ LiverpoolsNumber9 Bueno, entonces ¿por qué tener un lenguaje fuertemente tipado? Los errores del compilador son una primera línea de defensa contra errores. Las pruebas unitarias son las segundas. Las pruebas unitarias tampoco recogerían la tuya porque es una interacción entre una clase y su dependencia, por lo que estarías en la tercera línea de defensa: las pruebas de integración. No sé con qué frecuencia los ejecuta, pero ahora está hablando de comentarios en el orden de minutos o más, en lugar de los milisegundos que le toma a su IDE subrayar un error del compilador.
Ben Aaronson
2
@ LiverpoolsNumber9 incorrecto: sus pruebas ahora tienen que llenar IContextay inyectar eso, y de manera crucial: si agrega una nueva dependencia, el código aún se compilará , a pesar de que las pruebas fallarán en tiempo de ejecución
AlexFoxGill