5

I've just finished Mark Seemann's book Dependency Injection in .NET and I'm now trying to refactor some legacy code. (I am not, at this stage, relying on any particular DI container, rather just trying to move all the dependencies to one place).

I'm looking at the following factory class which determines the ArchiveType by reading the first few bytes of the archive with archiveReader.GetArchiveType() and then returns an instance of an ArchiveRestorer based on the ArchiveType enum.

public class ArchiveRestorerFactory : IArchiveRestorerFactory
{
    public ArchiveRestorer Create(ArchiveReader archiveReader)
    {
        ArchiveType type = archiveReader.GetArchiveType();
        switch (type)
        {
            case ArchiveType.CurrentData:
                return new CurrentDataArchiveRestorer(archiveReader);
                break;
            case ArchiveType.HistoricalData:
                return new HistoricalDataArchiveRestorer(archiveReader);
                break;
            case ArchiveType.AuditTrail:
                return new AuditTrailArchiveRestorer(archiveReader);
                break;
            default:
                throw new Exception("ArchiveRestorerFactory error: Unknown value for ArchiveType.");
        }
    }
}

How do I refactor this so that the class does not depend on the concrete types CurrentDataArchiveRestorer, HistoricalDataArchiveRestorer and AuditTrailArchiveRestorer?

Should I move the three concrete restorers into the factory's constructor?

public ArchiveRestorer Create(ArchiveReader archiveReader, 
    ArchiveRestorer currentDataArchiveRestorer, 
    ArchiveRestorer historicalDataArchiveRestorer, 
    ArchiveRestorer auditTrailDataArchiveRestorer)
{
    // guard clauses...
    // assign to readonly fields
}

That seems to be the approach suggested here, but then it will instantiate all three restorers when only one is needed? What if I had 20 different possible concrete implementations instead?

I feel like I should be implementing a concrete factory for each type of restorer and returning that instead but then I would just be replacing one new with another.

What is the best way to refactor this?

Community
  • 1
  • 1
shamp00
  • 11,106
  • 4
  • 38
  • 81
  • 1
    I think your scenario might be better suited to a chain of responsibility pattern. Look at [this example](http://davidhayden.com/blog/dave/archive/2008/11/19/ChainResponsibilityDesignPatternUnityDependencyInjectionContainer.aspx) for usage of the pattern in conjunction with a specific DI container (Unity). – Paolo Falabella Feb 02 '12 at 18:19
  • I would not implement the registration and assembly of the chain that way, but it is definitely a valid approach. – Sebastian Weber Feb 02 '12 at 18:52
  • Apart from the (redundant?) use of an `enum`, what particularly troubles you about your current implementation? – Mark Seemann Feb 02 '12 at 21:09
  • @MarkSeemann: I am bothered by the `new` keyword being used. According to your book (p. 136) "The *Control Freak* anti-pattern occurs every time we get an instance of a *dependency* by directly or indirectly using the `new` keyword in any place other than a *composition root*." – shamp00 Feb 03 '12 at 09:27
  • Well, you *could* put the factory implementation in the Composition Root, then... or you could resort to the second option you outline. If you have 20 different implementations, then consider injecting a list along with a Specification or predicate that enables you to select one instance from that list... or wrap them all into a Composite. – Mark Seemann Feb 03 '12 at 09:40
  • @MarkSeeman: I want to avoid instantiating all `ArchiveRestorer` subclasses when only one will be used. Unless I'm missing something, if I inject them all or if I use a Composite, all three will be instantiated. – shamp00 Feb 03 '12 at 10:29
  • @MarkSeemann Because a factory should not have to create an instance of every possible return type. In this particular case, it would not be expensive to create all three subclasses, but I am trying to find general guidelines for factories. In your book there is a similar example on p.167 where you use an `IRouteAlgorithmFactory`. However, you do not provide a concrete implementation of the `CreateAlgorithm(RouteType routeType)` (the source code provides only a mock). Is it good DI practice if the factory method uses `new`? If so, why? If not, what pattern can I use to avoid it? – shamp00 Feb 06 '12 at 09:48
  • http://blog.ploeh.dk/2011/03/04/ComposeObjectGraphsWithConfidence.aspx – Mark Seemann Feb 06 '12 at 11:01

5 Answers5

2

The way I'd do this, given the code you've already got, would be to create a factory for each of these objects which has a Create() method.

I'd have an interface for these factories also and have them inherit from a general factory interface.

You can then use the interfaces as a point for injection into your constructor.

Which would be called similar to this:

case ArchiveType.CurrentData:
                return _currentDateArchiveRestorerFactory.Create(archiveReader);
                break;

Alternatively, it might be better to have a single factory that creates an instance of a given type. Since all of these objects are restorers you could just create the instance based on the enum rather than a switch.

_restorerFactory.Create(ArchiveType.CurrentData);
Jamie Dixon
  • 53,019
  • 19
  • 125
  • 162
2

Why not make the ArchiveReader responsible for creating the appropriate ArchiveRestorer? Then the first iteration of the code would look like this:

public class ArchiveRestorerFactory : IArchiveRestorerFactory
{
    public ArchiveRestorer Create(ArchiveReader archiveReader)
    {
        ArchiveRestorer restorer = archiveReader.GetArchiveRestorer();
        return restorer;
    }
}

By then, it should be pretty obvious that the factory is redundant, so in the second iteration of the code you can throw it away let the consumers invoke the ArchiveReader directly.

Mark Seemann
  • 225,310
  • 48
  • 427
  • 736
  • That sounds like an excellent refactoring and I'll implement it, but it does not solve the dependency problem. It moves the dependencies to the `ArchiveReader` class. `ArchiveReader.GetArchiveRestorer()` would still need to `new` up the correct subclass of `ArchiveRestorer`. – shamp00 Feb 03 '12 at 10:18
  • 1
    Inject them into the ArchiveReader. It all boils down to how it selects the appropriate subtype. Since you haven't shared that, it's a little hard to answer. However, if you do decide to share that, it should be a new question here on SO. – Mark Seemann Feb 03 '12 at 10:49
  • Doesnt this both fail to solve the dependency problem (as mentioned in the comment above) and also break SRP? Im currently reading your book but am getting caught up with these things when trying to apply the theories to real code. – Steve Sep 16 '15 at 15:24
  • 1
    @Steve As given here, my answer has refactored the problem to one of selecting an appropriate Strategy base on a run-time value. There are plenty of ways to address that problem: [Metadata Role Hint](http://blog.ploeh.dk/2013/01/09/MetadataRoleHint), [Role Interface Role Hint](http://blog.ploeh.dk/2013/01/10/RoleInterfaceRoleHint), or (my favourite) [Partial Type Name Role Hint](http://blog.ploeh.dk/2013/01/11/PartialTypeNameRoleHint). – Mark Seemann Sep 16 '15 at 16:18
1

Make one interface with one method with returns type of that interface and let three archiver classes implement that interface and then in the create method the parameter type would be just the interface and it will return the required object by calling the method of interface you just created. So you don't need concrete type in create method.

Kamal Kr
  • 687
  • 1
  • 10
  • 22
0

I would solve this problem by agreeing on a naming convention and utilizing Unity's ability to name registrations. Example of this here: https://dannyvanderkraan.wordpress.com/2015/06/29/real-world-example-of-dependency-injection-based-on-run-time-values/

Danny van der Kraan
  • 5,344
  • 6
  • 31
  • 41
0
interface ILogger
{
    void Log(string data);
}

class Logger : ILogger
{
    .
    .
    .
}

At this point, you use an intermediate factory object to return the logger to be used within the component:

class MyComponent
{
  void DoSomeWork()
  {
    // Get an instance of the logger
    ILogger logger = Helpers.GetLogger();
    // Get data to log
    string data = GetData();

    // Log
    logger.Log(data);
  }
}

class Helpers
{
  public static ILogger GetLogger()
  {
    // Here, use any sophisticated logic you like
    // to determine the right logger to instantiate.

    ILogger logger = null;
    if (UseDatabaseLogger)
    {
        logger = new DatabaseLogger();
    }
    else
    {
        logger = new FileLogger();
    }
    return logger;
  }
}
class FileLogger : ILogger
{
    .
    .
    .
}

class DatabaseLogger : ILogger
{
    .
    .
    .
}
NoWar
  • 36,338
  • 80
  • 323
  • 498