6
\$\begingroup\$

In an effort to create ever-improving code, I've written a DelegateCommand that takes a generic type parameter to be acted upon. It's pretty trivial, so it should hopefully be easy to review.

In order to do this, I had to create two interfaces: IGenericCommand and IWindowsUniversalCommand.

/// <summary>
/// Requires the implementation of <code>System.Windows.Input.ICommand</code> as well.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IGenericCommand<T> : ICommand
{
    /// <summary>
    /// A generic version of <code>System.Windows.Input.ICommand.CanExecute(object)</code>.
    /// </summary>
    /// <param name="parameter"></param>
    /// <returns></returns>
    bool CanExecute(T parameter);
    /// <summary>
    /// A generic version of <code>System.Windows.Input.ICommand.Execute(object)</code>.
    /// </summary>
    /// <param name="parameter"></param>
    void Execute(T parameter);
}

/// <summary>
/// Requires the implementation of <code>System.Windows.Input.ICommand</code> as well.
/// </summary>
interface IWindowsUniversalCommand : ICommand
{
    /// <summary>
    /// Should refresh whether or not the <code>ICommand</code> can execute, or call <code>System.Windows.Input.ICommand.CanExecuteChanged</code>.
    /// </summary>
    void RaiseCanExecuteChanged();
}

Then I created an abstract class WindowsUniversalCommand which implements some of the basic logic needed for the command, to save from having to repeat it regularly. (Just in case I create other commands for Windows Universal Apps.)

public abstract class WindowsUniversalCommand : IWindowsUniversalCommand
{
    public abstract bool CanExecute(object parameter);
    public abstract void Execute(object parameter);

#if WINDOWS_UWP
    // The .NET version on Windows RT (Universal Apps) does not support the `CommandManager` trick below, so we just make a regular event and fire it.
    public event EventHandler CanExecuteChanged;
#else
    // Programmes that are not Windows Universal Apps support the `CommandManager` trick.
    public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    } 
#endif

    protected virtual void OnCanExecuteChanged(EventArgs e)
    {
        var handler = CanExecuteChanged;
        handler?.Invoke(this, e);
    }

    /// <summary>
    /// This method is required for Windows Universal Apps (WinRT). Non-WinRT programmes may simply ignore this method.
    /// </summary>
    public void RaiseCanExecuteChanged() => OnCanExecuteChanged(EventArgs.Empty);
}

The two abstract methods are required so that WindowsUniversalCommand can satisfy the ICommand interface.

Lastly, I have a DelegateCommand<T>.

public class DelegateCommand<T> : WindowsUniversalCommand, IGenericCommand<T>
{
    private readonly Func<T, bool> _canExecuteMethod;
    private readonly Action<T> _executeMethod;

    public DelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod = null)
    {
        _executeMethod = executeMethod;
        _canExecuteMethod = canExecuteMethod;
    }

    public override bool CanExecute(object parameter) => parameter is T ? CanExecute((T)parameter) : false;

    public override void Execute(object parameter) => Execute((T)parameter);

    public bool CanExecute(T parameter) => _canExecuteMethod == null || _canExecuteMethod.Invoke(parameter);

    public void Execute(T parameter) => _executeMethod?.Invoke(parameter);
}
\$\endgroup\$
4
\$\begingroup\$
public override bool CanExecute(object parameter) => parameter is T ? CanExecute((T)parameter) : false;

Beware of a lesser known gotcha. This may not work as expected for parameter that's not "really" a T (it's not in Ts inheritance tree), only has an explicit cast operator defined allowing it to be casted to T.

In that case while (T) parameter would return a T instance, CanExecute won't even be called because your is check would prevent this from happening.

Other than that, I'd have some remarks related to naming and code style (rather subjective, so treat them as suggestions or food for thought more than claims your code is incorrect)

public interface IGenericCommand<T> : ICommand

There's no use in putting "generic" in the name. Any C# programmer knows that <T> indicates generics by itself. Case in point: in .NET we've got IEnumerable and IEnumerable<T> - not IGenericEnumerable<T>. (So "drop the the" ;) )

/// <typeparam name="T"></typeparam>

/// <param name="parameter"></param>
/// <returns></returns>

In my opinion such empty tags are of no use, all they do is clutter the codebase.

private readonly Func<T, bool> _canExecuteMethod;
private readonly Action<T> _executeMethod;

It's a matter of taste, but here I would ditch the "Method" suffix, as these aren't necessarily "methods". Note that you didn't name the class MethodCommand. Why not just _canExecute and _onExecute?

public bool CanExecute(T parameter) => _canExecuteMethod == null || _canExecuteMethod.Invoke(parameter);

Since you're using the null safety operator already, this could be simplified to _canExecuteMethod?.Invoke(parameter) ?? true.

\$\endgroup\$
  • \$\begingroup\$ Just as well, I was trying to think of a compelling reason to leave CanExecute(object) as a try/catch, that is one. \$\endgroup\$ – Der Kommissar Apr 2 '16 at 18:07
3
\$\begingroup\$

A quick comment for you. You can remove the null checking if you supply a default Func for your functions:

public class DelegateCommand<T> : WindowsUniversalCommand, IGenericCommand<T>
{
    private readonly Func<T, bool> _canExecuteMethod;
    private readonly Action<T> _executeMethod;

    public DelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod = null)
    {
        _executeMethod = executeMethod ?? _ => { };
        _canExecuteMethod = canExecuteMethod ?? _ => true;
    }

Although, I think it might be better to throw if executeMethod is null - that seems like it would be a programming error.

Adding doc comments is great but you could add a bit more detail. For example:

Requires the implementation of System.Windows.Input.ICommand as well.

Requires it in addition to what? There's also a see tag that will generate a link to the given type/member's documentation (in the right tools, e.g. sandcastle).

<see cref="System.Windows.Input.ICommand" />

I'd suggest that you keep to the documentation style of ICommand:

/// <summary>
/// An interface that allows an application author to define a typed method to be invoked.
/// </summary>
/// <remarks>This is a generic version of <see cref="System.Windows.Input.ICommand" /></remarks>
\$\endgroup\$

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.