Architecture problem: use of dependency injection resulting in rubbish API

Question

I'm tring to create a class which does all sorts of low-level database-related actions but presents a really simple interface to the UI layer.

This class represents a bunch of data all within a particular aggregate root, retrieved by a single ID int.

The constructor takes four parameters:

public AssetRegister(int caseNumber, ILawbaseAssetRepository lawbaseAssetRepository, IAssetChecklistKctcPartRepository assetChecklistKctcPartRepository, User user)
{
  _caseNumber = caseNumber;
  _lawbaseAssetRepository = lawbaseAssetRepository;
  _assetChecklistKctcPartRepository = assetChecklistKctcPartRepository;
  _user = user;
  LoadChecklists();
}

The UI layer accesses this class through the interface IAssetRegister. Castle Windsor can supply the ILawbaseAssetRepository and IAssetChecklistKctcPartRepository parameters itself, but the UI code needs to supply the other two using an anonymous type like this:

int caseNumber = 1000;
User user = GetUserFromPage();
IAssetRegister assetRegister = Moose.Application.WindsorContainer.Resolve<IAssetRegister>(new { caseNumber, user});

From the API design point of view, this is rubbish. The UI layer developer has no way of knowing that the IAssetRegister requires an integer and a User. They need to know about the implementation of the class in order to use it.

I know I must have some kind of design issue here. Can anyone give me some pointers?

Use constructor injection only for the injectable dependencies. Move the manually assigned values to be method parameters or provide a method to set them after construction. — Morten Mertner, Mar 11 '11 at 11:06
yes, that edit made me laugh too. not sure if you should really 'mess with the authors stylistic personality' in that way :) — jim tollan, Mar 11 '11 at 12:52
it's not DI that results in rubbish API. it's SL you're using, and that's common smell. Don't do SL. — Krzysztof Kozmic, Mar 12 '11 at 00:03
I never meant that DI itself results in a rubbish API, only my (mis)use of it. — David, Mar 13 '11 at 20:19
Why do you say this is SL? Because of the use in code of WindsorContainer.Resolve()? How else would I do it? — David, Mar 13 '11 at 20:36

Steven · Answer 1 · 2011-03-11T12:27:42.683

3

Try separating the message from the behavior. Make a class that holds the data for the operation, and create a different class that contains the business logic for that operation. For instance, create this command:

public class RegisterAssetCommand
{
    [Required]
    public int CaseNumber { get; set; }

    [Required]
    public User Operator { get; set; }
}

Now define an interface for handling business commands:

public interface ICommandHandler<TCommand>
{
    void Handle(TCommand command);
}

Your presentation code will now look like this:

var command = new RegisterAssetCommand
{
    CaseNumber = 1000,
    Operator = GetUserFromPage(),
};

var commandHandler = WindsorContainer
    .Resolve<ICommandHandler<RegisterAssetCommand>);

commandHandler.Handle(command);

Note: If possible, move the responsibility of getting a commandHandler out of the presentation class and inject it into the constructor of that class (constructor injection again).

No you can create an implementation of the ICommandHandler<RegisterAssetCommand> like this:

public class RegisterAssetCommandHandler
    : ICommandHandler<RegisterAssetCommand>
{
    private ILawbaseAssetRepository lawbaseAssetRepository;
    private IAssetChecklistKctcPartRepository assetRepository;

    public RegisterAssetCommandHandler(
        ILawbaseAssetRepository lawbaseAssetRepository,
        IAssetChecklistKctcPartRepository assetRepository)
    {
        this.lawbaseAssetRepository = lawbaseAssetRepository;
        this.assetRepository = assetRepository;
    }

    public void Handle(RegisterAssetCommand command)
    {
        // Optionally validate the command

        // Execute the command
    }
}

Optionally, you could perhaps even leave the User out of the RegisterAssetCommand by injecting a IUserProvider in the RegisterAssetCommandHandler. The IUserProvider interface could have an GetUserForCurrentContext that the handler can call.

I hope this makes sense.

edited Mar 11 '11 at 12:27

answered Mar 11 '11 at 11:19

Steven

166,672
24
332
435

I'm sorry, I don't understand this code. I need more spoonfeeding. How do I get my IAssetRegister instance? – David Mar 11 '11 at 12:09
Your `IAssetRegister` is gone. It has been split into two classes: A `RegisterAssetCommand`, and a `ICommandHandler`. The code that you did put in the implementation of `IAssetRegister` should be put into the `Handle` method of the `RegisterAssetCommandHandler` class. – Steven Mar 11 '11 at 12:25
Oh, I think I see. You think the IAssetRegister simply executes some command. In fact, it's basically like an entity - it contains state. Having said that, I think the problem is one of design - I need a repository to give me the asset register. I can then supply the case number and User to the repository's Load method. – David Mar 11 '11 at 13:11
Ah I see. Having your entities depend on services is generally a bad idea, but I guess you already figured that out, because that's what your question is all about. – Steven Mar 11 '11 at 13:33
Steven, I sense you are a goldmine, but I can't dig your jargon. Entities depend on services??? Do I need to read domain driven design? – David Mar 11 '11 at 15:21
(Massive thanks for your time and effort in guiding me here though...) – David Mar 11 '11 at 15:25
1

Your blog looks awesome by the way. I will have a stab at it after a strooooong coffee! :) – David Mar 11 '11 at 15:27
By default, everything you inject into an other class is a service, such as `ILawbaseAssetRepository`. I'm talking in Dependency Injection jargon here. When you try to inject a repository into an entity, it tells me that your entity is having multiple responsibilities. Try to separate the responsibilities. Let an entity be an entity, and define the logic (the code) somewere else. When you want to mutate state, use commands and command handlers. When you just want to query, place that in separate class as well. – Steven Mar 11 '11 at 16:14
Thanks for taking the time to clarify. – David Mar 11 '11 at 17:00

Sergi Papaseit · Accepted Answer · 2011-03-11T13:29:54.597

2

As Morten points out, move the non injectable dependecies from the constructor call to the method(s) that actually need to use it,

If you have constructor paramters that can't (or are difficult to) be injected you won't be able to autmatically inject IAssetRegister into any class that needs it either.

You could always, of course, create a IUserProvider interface with a concrete implementation along these lines:

public class UserProvider : IUserProvider 
{
    // interface method
    public User GetUser() 
    {
        // you obviously don't want a page dependency here but ok...
        return GetUserFromPage();
    }
}

Thus creating another injectable dependency where there was none. Now you eliminate the need to pass a user to every method that might need it.

edited Mar 11 '11 at 13:29

answered Mar 11 '11 at 11:15

Sergi Papaseit

15,999
16
67
101

That UserProvider idea could solve several problems for me. Thank you. – David Mar 11 '11 at 13:12
Okay, I fixed the problem. I am using an IUserProvider type interface to get the current user (injected of course), and supplying the case number via a method call rather than the constructor. Now my API doesn't require any guesswork from the developer. Thank you all! – David Mar 11 '11 at 15:22
Oh, and the IUserProvider concept also solved this problem for me: http://stackoverflow.com/questions/5192529/how-to-get-nhibernate-isession-to-cache-entity-not-retrieved-by-primary-key – David Mar 11 '11 at 15:23
That's more than I could have hoped for ;) – Sergi Papaseit Mar 11 '11 at 15:25

Architecture problem: use of dependency injection resulting in rubbish API

2 Answers2

Linked