Dessa forma, estou escrevendo esse código é testável, mas há algo errado com isso?

13

Eu tenho uma interface chamada IContext. Para isso, não importa realmente o que faz, exceto o seguinte:

T GetService<T>();

O que esse método faz é examinar o contêiner DI atual do aplicativo e tentar resolver a dependência. Bastante padrão, eu acho.

No meu aplicativo ASP.NET MVC, meu construtor se parece com isso.

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

Portanto, em vez de adicionar vários parâmetros no construtor para cada serviço (porque isso será realmente irritante e demorado para os desenvolvedores que estendem o aplicativo), estou usando esse método para obter serviços.

Agora, parece errado . No entanto, a maneira como atualmente estou justificando isso é isso - eu posso zombar .

Eu posso. Não seria difícil zombar IContextpara testar o Controlador. Eu teria que de qualquer maneira:

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

        // etc etc
    }
}

Mas como eu disse, parece errado. Quaisquer pensamentos / abuso são bem-vindos.

LiverpoolsNumber9
fonte
8
Isso se chama Service Locator e eu não gosto. Há muitos textos sobre o assunto - consulte martinfowler.com/articles/injection.html e blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern para iniciantes.
Benjamin Hodgson
No artigo de Martin Fowler: "Eu sempre ouvi a reclamação de que esses tipos de localizadores de serviços são uma coisa ruim porque não são testáveis ​​porque você não pode substituí-los por implementações. Certamente você pode projetá-los mal para entrar nisso. problemas, mas você não precisa. Nesse caso, a instância do localizador de serviço é apenas um detentor de dados simples. Eu posso criar o localizador facilmente com implementações de teste dos meus serviços. " Você poderia explicar por que não gosta? Talvez em uma resposta?
LiverpoolsNumber9
8
Ele está certo, isso é um design ruim. É fácil: public SomeClass(Context c). Este código é bastante claro, não é? Afirma, that SomeClassdepende de a Context. Err, mas espere, isso não acontece! Ele depende apenas da dependência Xobtida do Contexto. Isso significa que toda vez que você fizer uma alteração Context, poderá quebrar SomeObject, mesmo que você tenha mudado apenas Contexts Y. Mas sim, você sabe que mudou apenas Ynão X, então tudo SomeClassbem. Mas escrever um bom código não é sobre o que você sabe, mas sobre o que o novo funcionário sabe quando olha para o seu código pela primeira vez.
valenterry
@DocBrown Para mim, foi exatamente isso que eu disse - não vejo a diferença aqui. Você pode explicar melhor?
valenterry
1
@ DocBrown Eu entendo o seu ponto agora. Sim, se o contexto dele é apenas um conjunto de todas as dependências, isso não é um design ruim. Pode ser uma má nomeação, mas isso também é apenas uma suposição. O OP deve esclarecer se existem mais métodos (objetos internos) do contexto. Além disso, discutir o código é bom, mas isso é como programadores. Troca de pilha, então, para mim, também devemos tentar ver "por trás" as coisas para melhorar o OP.
valenterry

Respostas:

5

Ter um em vez de muitos parâmetros no construtor não é a parte problemática desse design . Desde que sua IContextclasse não seja senão uma fachada de serviço , especificamente para fornecer as dependências usadas no MyControllerBasee não um localizador de serviço geral usado em todo o código, essa parte do código está IMHO ok.

Seu primeiro exemplo pode ser alterado para

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

isso não seria uma mudança substancial no design MyControllerBase. Se esse design for bom ou ruim, depende apenas do fato de você querer

  • certifique-se TheContext, SomeServicee AnotherServicesão sempre tudo inicializado com objetos fictícios, ou todas elas com objetos reais
  • ou, para permitir inicializá-los com diferentes combinações dos 3 objetos (o que significa que, nesse caso, você precisaria passar os parâmetros individualmente)

Portanto, o uso de apenas um parâmetro em vez de 3 no construtor pode ser totalmente razoável.

O que é problemático é IContextexpor o GetServicemétodo em público. IMHO você deve evitar isso, em vez disso, mantenha os "métodos de fábrica" ​​explícitos. Então, tudo bem implementar os métodos GetSomeServicee GetAnotherServicedo meu exemplo usando um localizador de serviço? IMHO que depende. Desde que a IContextclasse continue sendo apenas uma fábrica abstrata simples com o objetivo específico de fornecer uma lista explícita de objetos de serviço, isso é aceitável pelo IMHO. Fábricas abstratas são tipicamente apenas códigos de "cola", que não precisam ser testadas por unidade. No entanto, você deve se perguntar se, no contexto de métodos como GetSomeService, se realmente precisa de um localizador de serviço ou se uma chamada explícita ao construtor não seria mais simples.

Portanto, tenha cuidado, quando você se apega a um design em que a IContextimplementação é apenas um invólucro em torno de um GetServicemétodo público genérico , permitindo resolver quaisquer dependências arbitrárias por classes arbitrárias, tudo se aplica ao que @BenjaminHodgson escreveu em sua resposta.

Doc Brown
fonte
Eu concordo com isto. O problema com o exemplo é o GetServicemétodo genérico . Refatorar para métodos explicitamente nomeados e digitados é melhor. Melhor ainda seria explicitar explicitamente as dependências da IContextimplementação no construtor.
Benjamin Hodgson
1
@BenjaminHodgson: às vezes pode ser melhor, mas nem sempre é melhor. Você nota o cheiro do código quando a lista de parâmetros do ctor fica cada vez mais longa. Veja minha resposta anterior aqui: programmers.stackexchange.com/questions/190120/…
Doc Brown
@DocBrown o "cheiro de código" da injeção excessiva de construtor é indicativo de uma violação do SRP, que é o problema real. Simplesmente agrupar vários serviços em uma classe Facade, apenas para expô-los como propriedades, não faz nada para resolver a origem do problema. Assim, uma fachada não deve ser um invólucro simples em torno de outros componentes, mas idealmente deve oferecer uma API simplificada pelo qual a consumi-los (ou deve simplificá-los de alguma outra forma) ...
AlexFoxGill
... Lembre-se de que um "cheiro de código" como a injeção excessiva de construtores não é um problema em si. É apenas uma sugestão de que há um problema mais profundo no código que normalmente pode ser resolvido por refatoração sensivelmente uma vez que o problema foi identificado
AlexFoxGill
Onde você estava quando a Microsoft fez isso IValidatableObject?
precisa saber é o seguinte
15

Esse design é conhecido como Service Locator * e eu não gosto. Existem muitos argumentos contra:

O Localizador de serviços o acopla ao seu contêiner . Usando injeção de dependência regular (onde o construtor explicita explicitamente as dependências), você pode substituir diretamente seu contêiner por outro diferente ou voltar para newexpressões. Com o seu, IContextisso não é realmente possível.

O Localizador de serviço oculta dependências . Como cliente, é muito difícil dizer o que você precisa para construir uma instância de uma classe. Você precisa de algum tipo de IContext, mas também precisa configurar o contexto para retornar os objetos corretos para fazer o MyControllerBasetrabalho. Isso não é de todo óbvio a partir da assinatura do construtor. Com DI regular, o compilador informa exatamente o que você precisa. Se a sua turma tem muitas dependências, você deve sentir essa dor, porque isso o estimulará a refatorar. O Localizador de serviço oculta os problemas com designs ruins.

O Localizador de serviço causa erros em tempo de execução . Se você chamar GetServicecom um parâmetro de tipo ruim, receberá uma exceção. Em outras palavras, sua GetServicefunção não é uma função total. (Funções totais são uma ideia do mundo do FP, mas basicamente significa que as funções devem sempre retornar um valor.) Melhor deixar o compilador ajudar e informar quando você tem as dependências erradas.

O Localizador de serviço viola o princípio de substituição de Liskov . Como seu comportamento varia de acordo com o argumento de tipo, o Service Locator pode ser exibido como se tivesse efetivamente um número infinito de métodos na interface! Este argumento é explicado em detalhes aqui .

É difícil testar o Localizador de Serviço . Você deu um exemplo de falsificação IContextpara testes, o que é bom, mas certamente é melhor não ter que escrever esse código em primeiro lugar. Basta injetar suas dependências falsas diretamente, sem passar pelo localizador de serviços.

Em suma, apenas não faça isso . Parece uma solução sedutora para o problema de classes com muitas dependências, mas a longo prazo você apenas tornará sua vida miserável.

* Estou definindo o Service Locator como um objeto com um Resolve<T>método genérico capaz de resolver dependências arbitrárias e usado em toda a base de código (não apenas na raiz da composição). Não é o mesmo que Service Facade (um objeto que agrupa um pequeno conjunto conhecido de dependências) ou Abstract Factory (um objeto que cria instâncias de um único tipo - o tipo de Abstract Factory pode ser genérico, mas o método não é) .

Benjamin Hodgson
fonte
1
Você está em conformidade com o padrão do Localizador de serviço (com o qual concordo). Mas, na verdade, no exemplo do OP, MyControllerBasenão é acoplado a um contêiner de DI específico, nem esse "realmente" é um exemplo para o antipadrão do Localizador de Serviço.
Doc Brown
@DocBrown Eu concordo. Não porque isso facilita minha vida, mas porque a maioria dos exemplos fornecidos acima não é relevante para o meu código.
LiverpoolsNumber9
2
Para mim, a marca registrada do antipadrão do Localizador de Serviço é o GetService<T>método genérico . Resolver dependências arbitrárias é o cheiro real, presente e correto no exemplo do OP.
Benjamin Hodgson
1
Outro problema ao usar um Localizador de Serviço é que ele reduz a flexibilidade: você pode ter apenas uma implementação de cada interface de serviço. Se você criar duas classes que dependem de um IFrobnicator, mas depois decidir que uma deve usar sua implementação original do DefaultFrobnicator, mas a outra realmente deve usar um decorador de CacheingFrobnicator, é necessário alterar o código existente. injetando dependências diretamente, tudo o que você precisa fazer é alterar o código de configuração (ou o arquivo de configuração, se você estiver usando uma estrutura DI). Portanto, isso é uma violação do OCP.
Jules
1
@DocBrown O GetService<T>()método permite que classes arbitrárias sejam solicitadas: "O que esse método faz é olhar para o contêiner DI atual do aplicativo e tentar resolver a dependência. Penso que é bastante padrão". . Eu estava respondendo ao seu comentário na parte superior desta resposta. Isso é 100% um localizador de serviço
AlexFoxGill 15/01
5

Os melhores argumentos contra o antipadrão do Localizador de Serviço são claramente declarados por Mark Seemann, por isso não vou falar muito sobre por que essa é uma péssima idéia - é uma jornada de aprendizado que você precisa dedicar um tempo para entender por si mesmo (I também recomende o livro de Mark ).

OK, então, para responder à pergunta - vamos repor o seu problema real :

Portanto, em vez de adicionar vários parâmetros no construtor para cada serviço (porque isso será realmente irritante e demorado para os desenvolvedores que estendem o aplicativo), estou usando esse método para obter serviços.

Há uma pergunta que resolve esse problema no StackOverflow . Como mencionado em um dos comentários lá:

A melhor observação: "Um dos maravilhosos benefícios da Injeção de Construtor é que ela torna violentas as violações do Princípio da Responsabilidade Única".

Você está procurando no lugar errado a solução para o seu problema. É importante saber quando uma aula está fazendo muito. No seu caso, eu suspeito fortemente que não há necessidade de um "Controlador de Base". De fato, no POO quase sempre não há necessidade de herança . Variações no comportamento e na funcionalidade compartilhada podem ser obtidas inteiramente através do uso apropriado de interfaces, o que geralmente resulta em um código melhorado e encapsulado - e não é necessário passar dependências para os construtores da superclasse.

Em todos os projetos em que trabalhei onde existe um Controlador Base, isso foi feito exclusivamente para o compartilhamento de propriedades e métodos convenientes, como IsUserLoggedIn()e GetCurrentUserId(). PARAR . Isso é horrível mau uso da herança. Em vez disso, crie um componente que exponha esses métodos e dependa dele onde for necessário. Dessa forma, seus componentes permanecerão testáveis ​​e suas dependências serão óbvias.

Além de qualquer outra coisa, ao usar o padrão MVC, eu sempre recomendaria controladores magros . Você pode ler mais sobre isso aqui, mas a essência do padrão é simples: os controladores no MVC devem fazer apenas uma coisa: manipular argumentos transmitidos pela estrutura do MVC, delegando outras preocupações a algum outro componente. Esse é novamente o princípio de responsabilidade única em ação.

Seria realmente útil conhecer seu caso de uso para fazer um julgamento mais preciso, mas honestamente, não consigo pensar em nenhum cenário em que uma classe base seja preferível a dependências bem fatoradas.

AlexFoxGill
fonte
+1 - esta é uma nova visão sobre a questão que nenhuma das outras respostas realmente têm abordado
Benjamin Hodgson
1
"Um dos benefícios maravilhosos da injeção de construtores é que ela torna violentamente óbvias as violações do princípio de responsabilidade única" Adoro isso. Resposta realmente boa. Não concorde com tudo isso. Meu caso de uso é, curiosamente, não ter que duplicar o código em um sistema que terá mais de 100 controladores (pelo menos). No entanto, o SRP - cada serviço injetado tem uma única responsabilidade, assim como o controlador que usa todos eles - como refrataria isso ?!
LiverpoolsNumber9
1
@ LiverpoolsNumber9 A chave é mover a funcionalidade do BaseController para dependências, até que você não tenha mais nada no BaseController, exceto protecteddelegações de uma linha para os novos componentes. Em seguida, você pode perder BaseController e substituir os protectedmétodos com chamadas diretas para dependências - o que irá simplificar o seu modelo e fazer todas as suas dependências explícita (que é uma coisa muito boa em um projeto com 100s de controladores!)
AlexFoxGill
@ LiverpoolsNumber9 - se você pode copiar uma parte do seu BaseController a um pastebin eu poderia fazer algumas sugestões concretas
AlexFoxGill
-2

Estou adicionando uma resposta a isso com base nas contribuições de todos os outros. Muito obrigado a todos. Primeiro, aqui está a minha resposta: "Não, não há nada errado com isso".

Resposta do Doc Brown "Service Facade" Aceitei essa resposta porque o que estava procurando (se a resposta fosse "não") eram alguns exemplos ou alguma expansão do que estava fazendo. Ele forneceu isso sugerindo que A) tem um nome e B) provavelmente existem maneiras melhores de fazer isso.

Resposta do "Localizador de serviço" de Benjamin Hodgson Por mais que eu aprecie o conhecimento que adquiri aqui, o que tenho não é um "Localizador de serviço". É uma "Fachada de Serviço". Tudo nesta resposta está correto, mas não para as minhas circunstâncias.

Respostas da USR

Vou abordar isso com mais detalhes:

Você está dando muitas informações estáticas dessa maneira. Você está adiando as decisões para o tempo de execução, como muitas linguagens dinâmicas. Dessa forma, você perde a verificação estática (segurança), a documentação e o suporte de ferramentas (preenchimento automático, refatorações, localização de usos, fluxo de dados).

Não perco nenhuma ferramenta e não estou perdendo nenhuma digitação "estática". A fachada do serviço retornará o que eu configurei no meu contêiner de DI ou default(T). E o que ele retorna é "digitado". A reflexão é encapsulada.

Não vejo por que adicionar serviços adicionais como argumentos de construtor é um grande fardo.

Certamente não é "raro". Como estou usando um controlador de base , toda vez que precisar alterar seu construtor, talvez seja necessário alterar 10, 100, 1000 outros controladores.

Se você usar uma estrutura de injeção de dependência, nem precisará passar manualmente os valores dos parâmetros. Então, novamente, você perde algumas vantagens estáticas, mas não tantas.

Estou usando injeção de dependência. Essa é a questão.

E, finalmente, no comentário de Jules sobre a resposta de Benjamin, não estou perdendo nenhuma flexibilidade. É a minha fachada de serviço. Posso adicionar quantos parâmetros GetService<T>desejar para distinguir entre diferentes implementações, da mesma maneira que faria ao configurar um contêiner de DI. Por exemplo, eu poderia mudar GetService<T>()para GetService<T>(string extraInfo = null)contornar esse "problema em potencial".


De qualquer forma, obrigado novamente a todos. Tem sido realmente útil. Felicidades.

LiverpoolsNumber9
fonte
4
Eu discordo que você tem uma Fachada de Serviço aqui. O GetService<T>()método é capaz de (tentar) resolver dependências arbitrárias . Isso o torna um localizador de serviço, não uma fachada de serviço, como expliquei na nota de rodapé da minha resposta. Se você o substituísse por um pequeno conjunto de GetServiceX()/ GetServiceY()métodos, como sugere o @DocBrown, seria uma Fachada.
Benjamin Hodgson
2
Você deve ler e prestar muita atenção à última seção deste artigo sobre o Abstract Service Locator , que é essencialmente o que você está fazendo. Vi esse anti-padrão corromper um projeto inteiro - preste muita atenção à citação "isso tornará sua vida como desenvolvedor de manutenção pior, porque você precisará usar quantidades consideráveis ​​de força cerebral para entender as implicações de todas as alterações que você fizer" "
AlexFoxGill 14/01
3
Na digitação estática - você está perdendo o objetivo. Se eu tentar escrever código que não forneça uma classe usando DI com um parâmetro de construtor necessário, ele não será compilado. Isso é segurança estática em tempo de compilação contra problemas. Se você escrever seu código, fornecendo seu construtor com um iContext que não tenha sido corretamente configurado para fornecer o argumento de que realmente precisa, então ele vai compilar e falhar em tempo de execução
Ben Aaronson
3
@ LiverpoolsNumber9 Bem, então por que ter uma linguagem fortemente tipada? Erros do compilador são a primeira linha de defesa contra bugs. Os testes de unidade são o segundo. O seu também não seria escolhido pelos testes de unidade, porque é uma interação entre uma classe e sua dependência; portanto, você estaria na terceira linha de defesa: testes de integração. Não sei com que frequência você os executa, mas agora você está falando sobre feedback na ordem de minutos ou mais, em vez dos milissegundos que o IDE leva para sublinhar um erro do compilador.
precisa saber é o seguinte
2
@ LiverpoolsNumber9 errado: os testes agora têm de preencher um IContexte injetar isso, e crucialmente: se você adicionar uma nova dependência, o código ainda irá compilar - mesmo que os testes irão falhar em tempo de execução
AlexFoxGill