Is Josh Smith's implementation of the RelayCommand flawed?

Solution 1:

I've found the answer in Josh's comment on his "Understanding Routed Commands" article:

[...] you have to use the WeakEvent pattern in your CanExecuteChanged event. This is because visual elements will hook that event, and since the command object might never be garbage collected until the app shuts down, there is a very real potential for a memory leak. [...]

The argument seems to be that CanExecuteChanged implementors must only hold weakly to the registered handlers, since WPF Visuals are to stupid to unhook themselves. This is most easily implemented by delegating to the CommandManager, who already does this. Presumably for the same reason.

Solution 2:

I too believe this implementation is flawed, because it definitely leaks the weak reference to the event handler. This is something actually very bad.
I am using the MVVM Light toolkit and the RelayCommand implemented therein and it is implemented just as in the article.
The following code will never invoke OnCanExecuteEditChanged:

private static void OnCommandEditChanged(DependencyObject d, 
                                         DependencyPropertyChangedEventArgs e)
{
    var @this = d as MyViewBase;
    if (@this == null)
    {
        return;
    }

    var oldCommand = e.OldValue as ICommand;
    if (oldCommand != null)
    {
        oldCommand.CanExecuteChanged -= @this.OnCanExecuteEditChanged;
    }
    var newCommand = e.NewValue as ICommand;
    if (newCommand != null)
    {
        newCommand.CanExecuteChanged += @this.OnCanExecuteEditChanged;
    }
}

However, if I change it like this, it will work:

private static EventHandler _eventHandler;

private static void OnCommandEditChanged(DependencyObject d,
                                         DependencyPropertyChangedEventArgs e)
{
    var @this = d as MyViewBase;
    if (@this == null)
    {
        return;
    }
    if (_eventHandler == null)
        _eventHandler = new EventHandler(@this.OnCanExecuteEditChanged);

    var oldCommand = e.OldValue as ICommand;
    if (oldCommand != null)
    {
        oldCommand.CanExecuteChanged -= _eventHandler;
    }
    var newCommand = e.NewValue as ICommand;
    if (newCommand != null)
    {
        newCommand.CanExecuteChanged += _eventHandler;
    }
}

The only difference? Just as indicated in the documentation of CommandManager.RequerySuggested I am saving the event handler in a field.

Solution 3:

Well, according to Reflector it's implemented the same way in the RoutedCommand class, so I guess it must be OK... unless someone in the WPF team made a mistake ;)