Simplifying RelayCommand/DelegateCommand in WPF MVVM ViewModels

Question

If you're doing MVVM and using commands, you'll often see ICommand properties on the ViewModel that are backed by private RelayCommand or DelegateCommand fields, like this example from the original MVVM article on MSDN:

RelayCommand _saveCommand;
public ICommand SaveCommand
{
    get
    {
        if (_saveCommand == null)
        {
            _saveCommand = new RelayCommand(param => this.Save(),
                param => this.CanSave );
        }
        return _saveCommand;
    }
}

However, this is a lot of clutter, and makes setting up new commands rather tedious (I work with some veteran WinForms developers who balk at all this typing). So I wanted to simplify it and dug in a little. I set a breakpoint at the first line of the get{} block and saw that it only got hit when my app was first loaded--I can later fire off as many commands as I want and this breakpoint never gets hit--so I wanted to simplify this to remove some clutter from my ViewModels and noticed that the following code works the same:

public ICommand SaveCommand
{
    get
    {
        return new RelayCommand(param => this.Save(), param => this.CanSave );
    }
}

However, I don't know enough about C# or the garbage collector to know if this could cause problems, such as generating excessive garbage in some cases. Will this pose any problems?

Festal

Answer 1

I discovered that you need the original way from MSDN if you have multiple controls that invoke the same commands, otherwise each control will new its own RelayCommand. I didn't realize this because my app only has one control per command.

So to simplify the code in ViewModels, I'll create a command wrapper class that stores (and lazily instantiates) all the RelayCommands and throw it in my ViewModelBase class. This way users do not have to directly instantiate RelayCommand or DelegateCommand objects and don't need to know anything about them:

    /// <summary>
    /// Wrapper for command objects, created for convenience to simplify ViewModel code
    /// </summary>
    /// <author>Ben Schoepke</author>
    public class CommandWrapper
    {
    private readonly List<DelegateCommand<object>> _commands; // cache all commands as needed

    /// <summary>
    /// </summary>
    public CommandWrapper()
    {
        _commands = new List<DelegateCommand<object>>();
    }

    /// <summary>
    /// Returns the ICommand object that contains the given delegates
    /// </summary>
    /// <param name="executeMethod">Defines the method to be called when the command is invoked</param>
    /// <param name="canExecuteMethod">Defines the method that determines whether the command can execute in its current state.
    /// Pass null if the command should always be executed.</param>
    /// <returns>The ICommand object that contains the given delegates</returns>
    /// <author>Ben Schoepke</author>
    public ICommand GetCommand(Action<object> executeMethod, Predicate<object> canExecuteMethod)
    {
        // Search for command in list of commands
        var command = (_commands.Where(
                            cachedCommand => cachedCommand.ExecuteMethod.Equals(executeMethod) &&
                                             cachedCommand.CanExecuteMethod.Equals(canExecuteMethod)))
                                             .FirstOrDefault();

        // If command is found, return it
        if (command != null)
        {
            return command;
        }

        // If command is not found, add it to the list
        command = new DelegateCommand<object>(executeMethod, canExecuteMethod);
        _commands.Add(command);
        return command;
    }
}

This class is also lazily instantiated by the ViewModelBase class, so ViewModels that do not have any commands will avoid the extra allocations.

Festal

If you are really that worried about your memory usage, wouldn't it make sense to only instantiate commands that you are going to use? For example, if you have a VM that exposes 3 commands, and only one is ever used, just drop the rest. If you are using all of your VM commands (which you should be), then your system of lazy loading will use MORE memory and MORE processor time which defeats the purpose of optimizing in the first place, especially for embedded systems. – Manilla

Answer 2

This is exactly the same as if you would offer a - say integer - property that calculates some constant value. You can either calculate it for each call on the get-method or you can create it on the first call and then cache it, in order to return the cached value for later calls. So if the getter is called at most once, it does make no difference at all, if it is called often, you will lose some (not much) performance, but you won't get real trouble.

I personally like to abbreviate the MSDN-way like this:

RelayCommand _saveCommand;
public ICommand SaveCommand
{
  get
  {
    return _saveCommand ?? (_saveCommand = new RelayCommand(param => this.Save(),
                                                            param => this.CanSave ));
  }
}

Monaghan

Good call. I like your usage of the null coalesce operator. I always forget about that one. – Manilla

Answer 3

One thing that I do is let Visual Studio do the typing for me. I just created a code snippet that allows me to create a RelayCommand by typing

rc Tab Save Enter

rc is the code snippet shortcut tab loads the text you type what you want and it creates all the other wording.

Once you look at one code snippet and create your own you'll never go back :)

For more information on creating code snippets: http://msdn.microsoft.com/en-us/library/ms165394.aspx

Woeful

I don't know why nobody else has mentioned this. This is what I do and it is by far the best way to handle boilerplate code for commands. All of that lazy->lazy stuff doesn't actually save any time or code in the long run. – Manilla

Answer 4

Why don't you write just:

private readonly RelayCommand _saveCommand = new RelayCommand(param => this.Save(),
                param => this.CanSave );;

public ICommand SaveCommand { get { return _saveCommand; } }

Peripteral

I guess is to prevent to create an object without the need to, but I don't really know the memory it uses or other side effects of creating a Command... – Blackstock

We wanted lazy instantiation to save memory. I ended up adding some code to a ViewModelBase class that stores a List<> of RelayCommands and has a method called GetCommand(). So when we implement a ViewModel, all we need to do to create a command is create an ICommand property that calls GetCommand() with the execute and canExecute delegates, and the ViewModel implementer doesn't need to know anything about RelayCommand/Delegate command. – Festal

I won't start optimizing a Command because the memory footprint of a Command is too low. – Peripteral

Gotcha. I am working on an embedded system so I have to minimize memory usage, especially in heavily used base classes like our ViewModelBase. – Festal

Answer 5

When you expose the ICommand property on your viewmodel and it doesn't have a backing field, this is ok, just as long as you only bind to this field once.

The GetCommand method of CommandWrapper will return the command if it is created already.

Nembutal

Answer 6

When you expose the ICommand property on your viewmodel and it doesn't have a backing field, this is ok, just as long as you only bind to this field once. Basically, when your form loads and it performs the initial bindings, this is the only time it's going to access your command's get property.

There are plenty of times when you'll bind a command only a single time.

If you bind the same command to more than one control, then you'll need the backing field.

Merari