10
\$\begingroup\$

I'm working with AutoFac to do some DI. I think I've got a decent grip on things, but just ran into a question I had with my code and thought I'd check:

public class AutofacRegistrations
{
    public static void RegisterAndSetResolver()
    {
        // Create the container builder.
        var containerBuilder = new ContainerBuilder();

        // Register the Web API controllers.
        containerBuilder.RegisterApiControllers(Assembly.GetExecutingAssembly());

        //  Only generate one SessionFactory ever because it is expensive.
        containerBuilder.Register(x => new NHibernateConfiguration().Configure().BuildSessionFactory()).SingleInstance();

        //  Everything else wants an instance of Session per HTTP request, so indicate that:
        containerBuilder.Register(x => x.Resolve<ISessionFactory>().OpenSession()).InstancePerApiRequest();
        containerBuilder.Register(x => LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType)).InstancePerApiRequest();

        //  TODO: I don't THINK I should actually be making a DAO factory per API request. I think the whole point of a Factory is to make them once.
        containerBuilder.RegisterType<NHibernateDaoFactory>().As<IDaoFactory>().InstancePerApiRequest();
        containerBuilder.RegisterType<StreamusManagerFactory>().As<IManagerFactory>().InstancePerApiRequest();

        // Build the container.
        ILifetimeScope container = containerBuilder.Build();

        // Create the depenedency resolver.
        var dependencyResolver = new AutofacWebApiDependencyResolver(container);

        // Configure Web API with the dependency resolver.
        GlobalConfiguration.Configuration.DependencyResolver = dependencyResolver;
    }
}

You can see that I am registering an NHibernateDaoFactory and a StreamusManagerFactory as InstancePerApiRequest. This doesn't feel right to me because I think creating a Factory is supposed to be a relatively expensive endeavour and they should probably be .SingleInstance() similiar to the ISessionFactory.

However, my NHibernateDaoFactory has a dependency on Session:

...

public NHibernateDaoFactory(ISession session)
{
    if (session == null) throw new NullReferenceException("session");

    Session = session;
}

public IErrorDao GetErrorDao()
{
    return ErrorDao ?? (ErrorDao = new ErrorDao(Session));
}

....

and my ManagerFactory has a dependency on the DaoFactory:

...

public StreamusManagerFactory(ILog logger, IDaoFactory daoFactory)
{
    if (logger == null) throw new NullReferenceException("logger");
    if (daoFactory == null) throw new NullReferenceException("daoFactory");

    Logger = logger;
    DaoFactory = daoFactory;
}

public IErrorManager GetErrorManager()
{
    return ErrorManager ?? (ErrorManager = new ErrorManager(Logger, DaoFactory.GetErrorDao()));
}

...

This leaves me having to create new factories per request. Is this correct?

\$\endgroup\$
  • 2
    \$\begingroup\$ Per Design Patterns: Elements of Reusable Object-Oriented Software (the famous GoF book), An abstract factory is often used as a singleton. I don't know autofac very well (never used it), but I'm sure it has a way of specifying a "singleton" lifestyle (Ninject has it as "InSingletonScope"). \$\endgroup\$ – Mathieu Guindon Mar 12 '14 at 17:04
6
\$\begingroup\$

Just got one thing to say on your code:

Your Comments add close to no value!

your code is littered with comments and empty lines

//Create the Container Builder
var containerBuilder = new ContainerBuilder();

The comment is in fact almost exactly the same as the code below. And it's not only there, it's the same with every comment in the AutofacRegistrations you posted.


Other than that, if your factory has a dependency on Session you maybe did something wrong. Instead you could try to pass the Session to the already instantiated singleton factory, when calling the GetErrorDao() Function.

That would instead look something like that:

public IErrorDao GetErrorDao(Session session)
{
     return ErrorDao ?? (ErrorDao = new ErrorDao(session));
}

this would decouple both your NHibernateDaoFactory and your ManagerFactory and let them be free to use a singleton pattern instead. Which in turn greatly reduces the stress your application puts on the system.

You'd need to cascade this up the Dependency Chain, but that's a small price to pay to have a decoupled Factory Instance for all Requests / Sessions instead of one for each Session.
You'd thus need to modify your GetErrorManager too:

public IErrorManager GetErrorManager(Session session)
{
    return ErrorManager ?? (ErrorManager = new ErrorManager(Logger, DaoFactory.GetErrorDao(session)));
}
\$\endgroup\$
  • \$\begingroup\$ The comments were simply taken directly from the AutoFac documentation... code.google.com/p/autofac/wiki/WebApiIntegration but I appreciate your concern. I do understand that I should be speaking about 'Why' and not just being explicit, but I didn't feel the need to remove comments from other's documentation. I'll consider inverting control of Session all the way up, but I thought the whole point of a Factory was to obfuscate that logic from others. That said, it does look like bubbling it up would be quite beneficial. I'll work on it. \$\endgroup\$ – Sean Anderson Mar 18 '14 at 18:57
  • 3
    \$\begingroup\$ @Sean the point of a Factory is to have a way to instantiate classes of a certain Interface Type, without needing to know the Class itself / have to care which implementation it actually is. \$\endgroup\$ – Vogel612 Mar 18 '14 at 19:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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