7

Right now we use DI/IOC and when we need to pass extra parameters to a constructor we use a factory class e.g.

public class EmailSender 
{
    internal EmailSender(string toEmail, string subject,String body, ILogger emailLogger)
    {.....} 
}

public class EmailSenderFactory
{
    ILogger emailLogger;
    public EmailSenderFactory(ILogger emailLogger)
    { 
        this.emailLogger = emailLogger;
    }
    public EmailSender Create(string toEmail, string subject, string body) 
    {
        return new EmailSender(toEmail, subject, body, emailLogger);
    }
}

Now the problem with this is that we end up creating a whole lotta factory classes, and people don't always know to use them (they sometimes new them up themselves). What are the biggest negatives of coding the class like this:

public class EmailSender 
{
    EmailLogger logger = IoC.Resolve<ILogger>();
    internal EmailSender(string toEmail, string subject,String body)
    {.....} 
}

Pro: we now can use the constructor safely without needing a factory class Con: we have to reference the Service Locator (I'm not worried about testability, its easy to use a mock container as the backing service for the container).

Is there some big stinker of a reason out there why we shouldn't do this?

edit: after a bit of thought, I twigged that by having a private constructor, and by nesting the Factory class, I could keep the implementation and factory together, and prevent people from creating classes improperly, so the question has become somewhat moot. All the points points about SL being dirty, are of course true, so the solution below keeps me happy:

public class EmailSender 
{
    public class Factory
    {
        ILogger emailLogger;
        public Factory(ILogger emailLogger)
        { 
            this.emailLogger = emailLogger;
        }
        public EmailSender Create(string toEmail, string subject, string body) 
        {
            return new EmailSender(toEmail, subject, body, emailLogger);
        }
    }
    private EmailSender(string toEmail, string subject,String body, ILogger emailLogger)
    {
    } 
}
alexandrul
  • 12,856
  • 13
  • 72
  • 99
mcintyre321
  • 12,996
  • 8
  • 66
  • 103
  • Thanks for posting edits, I'm finding this thought process very useful as I think through the same challenges. – rohancragg Dec 17 '09 at 15:57
  • This is an excellent question, and one that I don't think has really been properly answered below. Perhaps I'm missing something, but I've only seen solutions that allow you to set extra constructor params centrally in much the same as resolving the interfaces centrally is performed. That's a very different situation to the one described above, where you want to be able to use the ctor with params as usual, but also take advantage of decoupling the ILogger object. I can only assume that this kind of usage renders such scenarios unsuitable for DI with ctors. – Xiaofu Feb 06 '10 at 13:51
  • 1
    I'm getting the impression that the DI form of IoC should really be reserved for true "service components" that don't require configuration in the way you want to do it above. Having said that, I'd be delighted to be shown otherwise! :) – Xiaofu Feb 06 '10 at 13:52

3 Answers3

14

yes - it is bad.

  • Why write all that code when you can have the framework do the work? All the IoC.Resolve() calls are superfluous and you shouldn't have to write them.
  • Another, even more important aspect, is that your components are tied to your service locator.

    You're now unable to instantiate them just like that - you need a completely set up service locator in place every time you need to use a component.

  • Last but, bot least - your SL code is sprinkled all over your codebase which is not a good thing, because when you want to change something, you have to look in multiple places.
Krzysztof Kozmic
  • 27,267
  • 12
  • 73
  • 115
  • The writing 'x = IoC.Resolve()' code issue is a non-issue to me - it saves me having to write a load of factory classes. The problem of having to change my SL code, or getting tied to an implementation is fairly minor too - especially if I use http://www.codeplex.com/CommonServiceLocator. The worst point is that the component is tied to a particular SL, meaning a caller couldn't change the dependencies, so I'll accept this as best answer. – mcintyre321 Oct 23 '09 at 09:16
  • 4
    You don't have to write factory classes! IoC container is a single configurable uberfactory and if you structure your code properly it is all you need – Krzysztof Kozmic Oct 23 '09 at 09:38
  • and you do want to pull services from some factory yourself (and you're using Windsor), you still can use TypedFactory facility that will implement your factory backed by the container http://www.castleproject.org/container/facilities/v1rc3/typedfactory/index.html or AutoGen http://code.google.com/p/castleautogen/ – Krzysztof Kozmic Oct 23 '09 at 09:45
  • 1
    IN the situation detailed above you have parameters that need to be passed at creation time (body, to, subject). Without an explicit factory class you end up with magic strings as the parameter names. – mcintyre321 Oct 23 '09 at 11:31
  • Than you can use The facilities I gave you links above. However I think you're doing it wrong. I'd just have an IEmailService and pass body, to, subject as parameter to its *method*, leaving the details of the implementation encapsulated, It's implementation is free to new up the EmailMessage or whatever needed inside, and I as a user am shielded from these implementation details. Plus I don't need any factory to do that, reaping benefits of Inversion of Control – Krzysztof Kozmic Oct 23 '09 at 11:50
  • 1
    In that case IEmailService you suggest sounds a lot like a factory to me! The example above was intended to be representative of an immutable class of some kind that needs constructor params. My question, more accurately restated, becomes "in the situation that you have an immutable class, that takes some parameters not known until creation time and some that are registered in a DI container, and you don't want to have any magic strings in codebase*, is it OK to use inline calls to a SL to avoid the cost of writing factories". *a by product of auto implemented factories – mcintyre321 Oct 23 '09 at 13:41
  • well, no. I still think it's an issue with your design. Can you show more real life scenario? Why do you need this class to be immutable? emailService.SendEmail(toEmail,subject, body); <-- that's how I would do it. no need for SL here. – Krzysztof Kozmic Oct 23 '09 at 14:02
  • maybe each of the emails is an entity and I need to persist what I sent for reporting purposes? I reckon we've gone a bit far with this discussion though, I think I should stop posting theoretical Qs! – mcintyre321 Oct 23 '09 at 14:21
  • Yeah, we're drifting away :) Anyway - usually it is possible (and preferred) to strive to achieve SL independence. In your sample, I certainly wouldn't like to persist my email message in the same place that sends it! Anyway- good luck :) – Krzysztof Kozmic Oct 23 '09 at 14:47
2

The largest reason I can think of (without just looking at issues with Service Locators in general) is that it is not what I as a user of your class would expect.

Meta discussion:
Some DI frameworks (Guice for example) will build the factory for you.

Some people advocate separating the "newable" from the "injectable".

Michael Lloyd Lee mlk
  • 14,561
  • 3
  • 44
  • 81
  • @mlk: only just spotted your "newable" link after commenting on the original question. +1 for that, very interesting. So for the OP's specific scenario, would you classify this as a "newable" rather than an "injectable", and therefore unsuited or unable to use DI? – Xiaofu Feb 06 '10 at 14:55
  • I don't really go with the whole "Injecting a logger" thing to be honest. I would see as a job for either AOP or to simply grabbing from the factory provided by the logging framework. EmailSender seams like an odd class to me. I would likely see this as two classes, EmailMessage - a newable and EmailService - an injectable. – Michael Lloyd Lee mlk Feb 08 '10 at 14:14
1

I'm not quite sure about this strong "it is bad" answer gave by Krzysztof. I think there's some trade-off and preference in there without absolutely categorizing them as bad or good.

  • I don't think it is more superfluous writing those IoC.Resolve() call than writing specific constructors or properties for the injection mechanism.
  • This one, i have to agree that you are tied to a service Locator and you have to set it up before instancing a class. However:
    • You can segregate your service locator with more specific interfaces. Thereby reducing the coupling with a huge service locator for every services of your system
    • Yeah, if you use DI mechanism, you will remove all those IoC.Resolve(), but you would still have to use a kind of container to instantiate your "main" services. The DI has to intercept those calls, no?
    • Your service locator could (should?) be "auto-configurable" or at least, pretty easy to set up.
  • See above the "segregate your service locator with more specific interfaces..." point.

I think using a service locator is indeed hiding your dependencies inside the class instead of exposing them through constructors. And it is an inconvenient in my opinion because you will not know your class is missing something until the service locator is called without being configured.

But the DI thing is not free of that kind of code darkness. When you use DI, it is really not obvious to understand how those dependencies just "appeared" (DI magic) in your constructor. By using a SL, you at least can see where those dependencies are coming from.

But still, when testing a class that expose those dependencies on her constructors, you (almost) can't miss it. That is not the case using a service locator.

I'm not saying Krzysztof was wrong, because I agree with him for the most. But I'm pretty sure using a service locator is not necessarily a bad "design" and certainly not simply bad.

Phil

Phil Good
  • 106
  • 2
  • 8
  • 2
    When you look at this from the perspective of migrating a legacy code base towards (eventually) using DI then perhaps this is a (temporary) tool that can be used along the way? – rohancragg Dec 17 '09 at 15:49