Is this the right way to use the Command Pattern?

Question

Related to this other question: How to inject an action into a command using Ninject?

Based on the comments on the above-referenced question, I take it that I would just need to create some command classes and inject them in my view model so that the view's controls just need to bind to them. I conceptually agree and understand the benefits. Besides, I wish to be as clean as possible using Ninject, DI and Constructor Injection.

Following these important rules, here's what I've come with so far.

CreateCategoryCommand

public class CreateCategoryCommand : ICommand {
    public CreateCategoryCommand(CreateCategoryView view) {
        if(view == null) throw new ArgumentNullException("view");
        this.view = view;
    }

    public bool CanExecute(object parameter) { return true; }

    public event EventHandler CanExecuteChanged;

    public void Execute(object parameter) { view.Show(); }

    private readonly CreateCategoryView view;
}

CategoriesManagementViewModel

public class CategoriesManagementViewModel {
    public CategoriesManagementViewModel(ICommand createCommand) {
        if (createCommand == null) throw new ArgumentNullException("createCommand");
        this.createCommand = createCommand;
    }

    public ICommand CreateCommand { get { return createCommand; } }

    private readonly ICommand createCommand;
}

So now when the CategoriesManagementView is initialized, it is constructor-injected with the CategoriesManagementViewModel, which in turn is constructor-injected with the CreateCategoryCommand, which in turn is constructor-injected with the CreateCategoryView, so no redundant dependency, neither any cycle-dependency.

Now, when I the CategoriesManagementView.CreateButton, it shall trigger the bound CategoriesManagementViewModel.CreateCommand, which will show the CreateCategoryView to the user, and this view shall have its own proper commands as well injected the same way.

Finally, this would render the RelayCommand class as useless...

Is that it?

Answer 1

First, I agree that RelayCommand and DelegateCommand and the like are ways of implementing commands that violate SOLID principles, so your solution here to replace them with a separate class is the correct one. Doing so also keeps your ViewModels much cleaner.

That said, you're violating MVVM pretty badly by having a class in your ViewModels layer (the CreateCategoryCommand) have knowledge of a concrete that is in your Views layer (CreateCategoryView). Nothing in your ViewModels layer should have a direct reference to anything in your Views layer.

Imagine it this way - you've separated your layers out into different dlls - Views.dll, ViewModels.dll, Models.dll, DataLayer.dll. If something in your ViewModels has a reference to a concrete in your Views, and obviously your Views will have a reference to ViewModels (as is necessary), then you have a circular reference problem.

The solution is to have your View object implement an interface (Interface Segregation Principle) like IDialog or IUiDisplay (choose the name depending on how abstract you want to be), and have your command have a dependency on that interface, NOT the direct concrete type, like so:

In Views:

public class CreateCategoryView : ..., IUiDisplay
{
    ...
}

In ViewModels:

public interface IUiDisplay
{
    void Show();
}

public class CreateCategoryCommand : ICommand 
{
    public CreateCategoryCommand(IUiDisplay uiDisplay) {
        if(display == null) throw new ArgumentNullException("uiDisplay");
        this.display = uiDisplay;
    }

    private readonly IUiDisplay display;
    ...
}

Now, your Command no longer has a direct dependency on a concrete (so it is now mockable and testable!) from a higher layer. Now you can have your DI/IOC resolve the command dependency to the specific view class you want to inject. (I'd personally inject a view factory into the command instead, and only create the view lazily, but that's a different discussion).

One related note - if you implement commands by directly having them implement ICommand, then you're going to repeat yourself a lot (DRY). My suggestion is to create an abstract base class (CommandBase or something) that implements the requirements of ICommand. You'll find that all your commands that derive from it will only override Execute() and sometimes CanExecute(). This saves you from having to implement the event (and code to raise the event) in every command, and in many cases saves you from having to implement CanExecute since most commands just return true.