3
\$\begingroup\$

I had a rather large method in one public class that I refactored into 2 helper classes. The thing is though, that those 2 helper classes have dependencies. I refactored them into helper classes so I could mock and test them easily, which worked out perfectly.

However, the thing is I don't want to have to register my helper classes in the DI container, because I know the public class will always be using those specific implementations.

This is how I implemented the public class' constructors:

    /// <summary>
    /// Internal constructor used by tests for mocking.
    /// </summary>
    internal TranslationCompiler(ITranslationCatalogTransformer translationCatalogTransformer, ICompiledCatalogTransformer compiledCatalogTransformer)
    {
        if (compiledCatalogTransformer == null) throw new ArgumentNullException("compiledCatalogTransformer");
        if (translationCatalogTransformer == null) throw new ArgumentNullException("translationCatalogTransformer");
        _translationCatalogTransformer = translationCatalogTransformer;
        _compiledCatalogTransformer = compiledCatalogTransformer;
    }

    /// <summary>
    /// Public constructor that passes dependencies to concrete implementations of helper classes.
    /// </summary>
    public TranslationCompiler(IResourceService resourceService, ITranslationSerializer serializer)
    {
        if (resourceService == null) throw new ArgumentNullException("resourceService");
        if (serializer == null) throw new ArgumentNullException("serializer");
        _translationCatalogTransformer = new TranslationCatalogTransformer(resourceService);
        _compiledCatalogTransformer = new CompiledCatalogTransformer(serializer);
    }

Is this an acceptable use for poor man's DI? This way, the DI container only has to know the actual dependencies for the public class to work, while still being very testable.

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

I don't see anything wrong with your implementation - if it works for you, and does not interfere with the system - go for it!

I will even go further and claim that the guard conditions in your internal constructor are redundant because the constructor will only be used by the single public constructor, or by a test which:

  1. Either counts on the helper - in which case a NullPointerException will occur and fail the test, or
  2. Does not need the helper, in which case there is no need to mock it...

Final note: I guess that the public constructor actually uses resourceService and serializer in some code you deleted from it, otherwise, the guard conditions there are also redundant...

\$\endgroup\$
2
  • \$\begingroup\$ The public constructor just passes the dependencies along. :) \$\endgroup\$
    – Jeff
    Apr 25, 2014 at 11:43
  • 1
    \$\begingroup\$ With regards to your first point, such guards are considered best practice because they throw the exception as early as possible, thereby decreasing the number of opportunities for confusion. The problem is immediately indicated without debugging. This may be less important if the library is not reused, but that is never the right assumption. \$\endgroup\$
    – Magus
    Apr 25, 2014 at 14:29
3
\$\begingroup\$

Couple of things..

First, having nearly identical code in both constructors is a maintenance pain. It may be okay every so often, but if you do it a lot it will cause a bug one day when you change one and not the other.

You could change the public constructor to call the internal constructor like this:

public TranslationCompiler(IResourceService resourceService, ITranslationSerializer serializer)
    : this(new TranslationCatalogTransformer(resourceService), new CompiledCatalogTransformer(serializer))
{
}

Second, it's not really ideal to be creating different constructors (or any other code) just for tests. You really should be mocking the interfaces of the public API instead. In this case the IResourceService and the ITranslationSerializer.

If you are having trouble mocking those interfaces it's probably an indicator of some other issue with your design. Perhaps those interfaces can be broken up into a few smaller interfaces, or maybe your unit tests are making the wrong assumptions.

Maybe posting one of your unit tests would help.

\$\endgroup\$
3
\$\begingroup\$

However, [...] I don't want to have to register my helper classes in the DI container, because I know the public class will always be using those specific implementations.

That is a very strong assumption you're making here. Are you sure you'll never want to inject a decorator that will, say, report execution time to the debug output? Or one that catches and logs (with email notification if you want it) whatever uncaught exceptions that this other code could throw?

This is what proper DI allows you to do, when you inject dependencies as abstractions - the class has no clue of the actual implementing type, and doesn't care about it, because as long as the implementing type in question implements the specified interface, how it's implemented is not a concern of that class. It's actually none of its business.

/// <summary>
/// Public constructor that passes dependencies to concrete implementations of helper classes.
/// </summary>
public TranslationCompiler(IResourceService resourceService, ITranslationSerializer serializer)
{
    if (resourceService == null) throw new ArgumentNullException("resourceService");
    if (serializer == null) throw new ArgumentNullException("serializer");
    _translationCatalogTransformer = new TranslationCatalogTransformer(resourceService);
    _compiledCatalogTransformer = new CompiledCatalogTransformer(serializer);
}

The key to a successful implementation, is going all the way through.

You haven't listed your class, but I'm expecting to see this somewhere like right above these constructors you've listed:

private readonly ITranslationCatalogTransformer _translationCatalogTransformer;
private readonly ICompiledCatalogTransformer _compiledCatalogTransformer;

These are your class' dependencies. Inversion of Control specifically implies giving away that control you're keeping all for yourself - the control over the specific types that implement the abstractions you're depending on.

Just let go, embrace DI in its full glory.

If there aren't any other dependencies (are you newing up anything else anywhere else?), then at the end this should be the only constructor you need:

public TranslationCompiler(ITranslationCatalogTransformer translationCatalogTransformer,
                           ICompiledCatalogTransformer compiledCatalogTransformer)
{
    if (compiledCatalogTransformer == null) 
        throw new ArgumentNullException("compiledCatalogTransformer");

    if (translationCatalogTransformer == null) 
        throw new ArgumentNullException("translationCatalogTransformer");

    _translationCatalogTransformer = translationCatalogTransformer;
    _compiledCatalogTransformer = compiledCatalogTransformer;
}

Even if you actually end up using a specific implementation forever, that's not a reason to introduce tight coupling in your architecture.


What you have here isn't Poor Man's DI. It's not DI.

You depend on two classes that each have their own dependencies, that you're able to provide via an IoC container: there's no reason to new them up yourself, unless there's lack of context in your post or, more likely, unless there's something I missed.

The public constructor you have, doesn't take the constructed type's dependencies, and this only uselessly blurs things up. Poor Man's DI is a replacement for an IoC container, not a reason to introduce tight coupling.

You're injecting your dependencies' dependencies: this means your client code must new up things that the object being created doesn't even need. Doesn't it smell?

Even if you are going to always be using those specific implementations, by newing them up yourself you have ruled out all the advantages of doing DI in the first place, and you have made your constructors much more ambiguous and complex than they need to be, at least from a DI standpoint, where a type's constructor signature is expected to tell us what that type's dependencies are.

...which brings us to tell, don't ask.

\$\endgroup\$
1
  • \$\begingroup\$ The reason I did it like this was to not have everyone know about my helper classes which are only there to avoid having a huge method in my class. The alternative was to refactor them into methods instead of helper classes, and make them internal virtual so I could mock them. But good point. \$\endgroup\$
    – Jeff
    May 1, 2014 at 6:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.