Best Practice Question

Topics: Customizing Orchard, Writing modules
Dec 14, 2011 at 3:07 PM
Edited Dec 14, 2011 at 3:12 PM

Which way is better / preferred:

Getting references to the ContentManager and TransactionManager via the OrchardService object -

private readonly IOrchardServices _services;
private readonly IContentManager _contentManager;
private readonly ITransactionManager _transactionManager;

public AdminController(IOrchardServices services) {
  _services = services;
  _contentManager = _services.ContentManager;
  _transactionManager = _services.TransactionManager;
}

 or

Getting references to the ContentManager and TransactionManager via dependency injection -

private readonly IOrchardServices _services;
private readonly IContentManager _contentManager;
private readonly ITransactionManager _transactionManager;

public AdminController(IOrchardServices services,
IContentManager contentManager,
ITransactionManager transactionManager) { _services = services; _contentManager = contentManager; _transactionManager = transactionManager; }

Thanks!

Coordinator
Dec 15, 2011 at 1:22 AM

Actually, I would do neither. Just inject services and get content manager and transaction manager as needed.

Dec 15, 2011 at 1:38 AM
Edited Dec 15, 2011 at 1:40 AM

OK.  I do inject the services needed I just left them out.

Thanks for all your help!  I promise my questions will become much less silly! :)

Developer
Dec 15, 2011 at 8:49 AM

@Bertrand: Is there a reason not to save a reference e.g. for ContentManager in the ctor? I sometimes do this for convenience reasons.

Coordinator
Dec 15, 2011 at 5:13 PM

My reason is that I don't see the point: you already have services. It's one dot away.

Dec 15, 2011 at 5:26 PM

What you'll sometimes see in Orchard core modules is that IOrchardServices is one of the few dependencies always stored on a public property "Services". (You'll notice Logger and T are usually also public)

I think the reason for this lies in tests. You can swap in a different IOrchardServices implementation for testing, so obviously if you stored the others in the constructor then you wouldn't be able to swap out those implementations.

Coordinator
Dec 15, 2011 at 5:33 PM

That's a good point: explicitly getting all three as injected dependencies introduces the possibility of mismatches if parts of the code is getting them out of services and parts is getting it directly (and that code may be deep in what your code calls into).

Developer
Dec 15, 2011 at 7:05 PM

What I've meant by "saving a reference" (in contrast to what in the opening post is) is injecting only an IContentManager in the constructor and assigning the actually used services to member variables, like:

public MyClass(IOrchardServices orchardServices)
{
    _contentManager = orchardServices.ContentManager;
    _notifier = orchardServices.Notifier;
}
Just for the sole purpose of not having _orchardServices.ContentManager.WhatEver() calls, but _contentManager.WhatEver().

Coordinator
Dec 15, 2011 at 8:15 PM

ah yes, sure, that's perfectly fine.

Dec 15, 2011 at 10:55 PM

It's still the same thing I meant, swapping in a mock IOrchardServices will be difficult (if you use the same testing pattern as Orchard core modules).

I think this way would be preferable:

private IContentManager _contentManager { get { return Services.ContentManager; } }

Developer
Dec 16, 2011 at 8:55 AM

But why would it be more difficult? I don't understand. You have to provide a mock for IOrchardServices and its services anyway when instantiating the class to be tested. Why would you swap the IContentManager instance after you've instantiated the class? Or am I misunderstanding?

With ILogger and Localizer I think the cause why they're stored in a public property is that they can't be injected through constructor arguments.

Dec 16, 2011 at 2:09 PM

No, in the unit tests I've seen the mock is set after instantiation. I have no idea why that is!

Developer
Dec 16, 2011 at 2:42 PM

That's strange. Maybe a core team member can explain it.