views:

385

answers:

2

I think this is the question, anyway. I am using a RelayCommand, which decorates an ICommand with two delegates. One is Predicate for the _canExecute and the other is Action for the _execute method.

---Background motivation --

The motivation has to do with unit testing ViewModels for a WPF presentation. A frequent pattern is that I have one ViewModel that has an ObservableCollection, and I want a unit test to prove the data in that collection is what I expect given some source data (which also needs to be converted into a collection of ViewModels). Even though the data in both collections looks the same in the debugger, it looks like the test fails due to an equality failure on the ViewModel's RelayCommand. Here's an example of the failing unit test:

[Test]
    public void Creation_ProjectActivities_MatchFacade()
    {
        var all = (from activity in _facade.ProjectActivities
                   orderby activity.BusinessId
                   select new ActivityViewModel(activity, _facade.SubjectTimeSheet)).ToList();

        var models = new ObservableCollection<ActivityViewModel>(all);
        CollectionAssert.AreEqual(_vm.ProjectActivities, models);
    }

--- Back to delegate equality ----

Here is the code for the RelayCommand - it's basically a direct rip-off of Josh Smith's idea, with an implementation for equality that I added in an attempt to solve this issue:

public class RelayCommand : ICommand, IRelayCommand
{
    readonly Action<object> _execute;
    readonly Predicate<object> _canExecute;

    /// <summary>Creates a new command that can always execute.</summary>
    public RelayCommand(Action<object> execute) : this(execute, null) { }

    /// <summary>Creates a new command which executes depending on the logic in the passed predicate.</summary>
    public RelayCommand(Action<object> execute, Predicate<object> canExecute) {
        Check.RequireNotNull<Predicate<object>>(execute, "execute");

        _execute = execute;
        _canExecute = canExecute;
    }

    [DebuggerStepThrough]
    public bool CanExecute(object parameter) { return _canExecute == null ? true : _canExecute(parameter); }

    public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }

    public void Execute(object parameter) { _execute(parameter); }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != typeof(RelayCommand)) return false;
        return Equals((RelayCommand)obj);
    }

    public bool Equals(RelayCommand other)
    {
        if (ReferenceEquals(null, other)) return false;
        if (ReferenceEquals(this, other)) return true;
        return Equals(other._execute, _execute) && Equals(other._canExecute, _canExecute);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return ((_execute != null ? _execute.GetHashCode() : 0) * 397) ^ (_canExecute != null ? _canExecute.GetHashCode() : 0);
        }
    }

}

In a unit test where I've effectively set the _execute delegate to the same method (_canExecute is null in both cases), the unit test fails at this line:

return Equals(other._execute, _execute) && Equals(other._canExecute, _canExecute)

Debugger output:

?_execute
{Method = {Void <get_CloseCommand>b__0(System.Object)}}
base {System.MulticastDelegate}: {Method = {Void CloseCommand>b__0(System.Object)}}

?other._execute
{Method = {Void <get_CloseCommand>b__0(System.Object)}} 
base {System.MulticastDelegate}: {Method = {Void CloseCommand>b__0(System.Object)}}

Can anyone explain what I am missing and what the fix is?

---- EDITED REMARKS ----

As Mehrdad pointed out, the get_CloseCommand from the debug session looks a bit weird at first. It really is just a property get, but it does raise the point as to why the equality of the delegate is problematic if I need to do tricks to make it work.

Some of the point of MVVM is to expose whatever might be useful in a presentation as properties, so you can use WPF binding. The particular class I was testing has a WorkspaceViewModel in it's heirarchy, which is just a ViewModel that already has a close command property. Here is the code:

public abstract class WorkspaceViewModel : ViewModelBase {

    /// <summary>Returns the command that, when invoked, attempts to remove this workspace from the user interface.</summary>
    public ICommand CloseCommand
    {
        get
        {
            if (_closeCommand == null)
                _closeCommand = new RelayCommand(param => OnRequestClose());

            return _closeCommand;
        }
    }
    RelayCommand _closeCommand;

    /// <summary>Raised when this workspace should be removed from the UI.</summary>
    public event EventHandler RequestClose;

    void OnRequestClose()
    {
        var handler = RequestClose;
        if (handler != null)
            handler(this, EventArgs.Empty);
    }

    public bool Equals(WorkspaceViewModel other) {
        if (ReferenceEquals(null, other)) return false;
        if (ReferenceEquals(this, other)) return true;
        return Equals(other._closeCommand, _closeCommand) && base.Equals(other);
    }

    public override int GetHashCode() {
        unchecked {
            {
                return (base.GetHashCode() * 397) ^ (_closeCommand != null ? _closeCommand.GetHashCode() : 0);
            }
        }
    }
}

You can see that the close command is a RelayCommand, and that I monkeyed with equals to make the unit test work.

@Merhdad Here is the unit test that only works when I use Trickster's delegate.Method in the equality comparison.

[TestFixture] public class WorkspaceViewModelTests { private WorkspaceViewModel vm1; private WorkspaceViewModel vm2;

    private class TestableModel : WorkspaceViewModel
    {

    }

    [SetUp]
    public void SetUp() {
        vm1 = new TestableModel();
        vm1.RequestClose += OnWhatever;
        vm2 = new TestableModel();
        vm2.RequestClose += OnWhatever;
    }

    private void OnWhatever(object sender, EventArgs e) { throw new NotImplementedException(); }


    [Test]
    public void Equality() {
        Assert.That(vm1.CloseCommand.Equals(vm2.CloseCommand));
        Assert.That(vm1.Equals(vm2));
    }


}

----- LATEST EDITS TO USE MERHDAD"S IDEA

debugger out put ?valueOfThisObject {Smack.Wpf.ViewModel.RelayCommand} base {SharpArch.Core.DomainModel.ValueObject}: {Smack.Wpf.ViewModel.RelayCommand} _canExecute: null _execute: {Method = {Void _executeClose(System.Object)}}

?valueToCompareTo
{Smack.Wpf.ViewModel.RelayCommand}
base {SharpArch.Core.DomainModel.ValueObject}: {Smack.Wpf.ViewModel.RelayCommand}
_canExecute: null
_execute: {Method = {Void _executeClose(System.Object)}}

?valueOfThisObject.Equals(valueToCompareTo)
false

This is the result after changing the code to:

    public ICommand CloseCommand
    {
        get
        {
            if (_closeCommand == null)
                _closeCommand = new RelayCommand(_executeClose);

            return _closeCommand;
        }
    }
    RelayCommand _closeCommand;

    void _executeClose(object param) {
        OnRequestClose();
    }
+1  A: 

I don't know anything now about other lines but what if

CollectionAssert.AreEqual(_vm.ProjectActivities, models);

fails just because ReferenceEquality is used?

You have overridden the comparison for RelayCommand but not for ObservableCollection.

And it looks like in case of Delegates Reference equality is used too.

Try to compare by Delegate.Method instead.

Trickster
The CollectionAssert is from nunit framework; as I understand it it's just forcing an equality test on each element in each collection (plus a check that the order is the same too). I gave you a point since Delegate.Method does work, but according the rules this shouldn't be necessary. Makes me think there is a better answer out there.
Berryl
What the rules? :-)
Trickster
The main rule is whatever works, but I was referring to C# specification (§7.9.8) since Merhdad was kind enough to post it here! Cheers :-)
Berryl
don't forget to mark the answer =).
Trickster
+4  A: 

Are you creating the delegate out of anonymous functions or something? These are the exact delegate equality rules according to C# specification (§7.9.8):

Delegate equality operators

Two delegate instances are considered equal as follows: If either of the delegate instances is null, they are equal if and only if both are null.
If the delegates have different runtime type they are never equal. If both of the delegate instances have an invocation list (§15.1), those instances are equal if and only if their invocation lists are the same length, and each entry in one’s invocation list is equal (as defined below) to the corresponding entry, in order, in the other’s invocation list. The following rules govern the equality of invocation list entries:
If two invocation list entries both refer to the same static method then the entries are equal.
If two invocation list entries both refer to the same non-static method on the same target object (as defined by the reference equality operators) then the entries are equal.
Invocation list entries produced from evaluation of semantically identical anonymous-function-expressions with the same (possibly empty) set of captured outer variable instances are permitted (but not required) to be equal.

So, in your case, it's possible that the delegate instances are referring to the same method in two different objects, or referring to two anonymous methods.


UPDATE: Indeed, the problem is that you are not passing the same method reference when you are calling new RelayCommand(param => OnCloseCommand()). After all, the lambda expression specified here is actually an anonymous method (you are not passing a method reference to OnCloseCommand; you are passing a reference to an anonymous method which takes a single parameter and calls OnCloseCommand). As mentioned in the last line of the specification quotation above, it's not necessary that comparing those two delegates return true.

Side Note: The getter of the CloseCommand property would be simply called get_CloseCommand and not <get_CloseCommand>b__0. This is the compiler generated method name for the anonymous method inside get_CloseCommand method (the CloseCommand getter). This further proves the point I mentioned above.

Mehrdad Afshari
In this case, both delegate instances are referring to the same object, and neither is an anonymous method. Trickster's idea to compare the Delegate.Method does work, although the rules would suggest I shouldn't have to do that.
Berryl
Berryl: If it's not an anonymous method, why it's named so weirdly? "`<get_CloseCommand>b__0`"
Mehrdad Afshari
Hi Mehrdad. That would just be the expansion of getting the CloseCommand property. I'll edit my post and elaborate a bit more.
Berryl
Do you also think testing Delegate.Method is the best thing to do here then?
Berryl
No. Nothing in the spec guarantees that the two delegates should be mapped to a single method. Also, by comparing `Delegate.Method`, you will deliberately *ignore* the equality of target objects. I'd write a method instead of the anonymous method (`void X(object param) { OnCloseCommand(); }`) and use that in `new ReplyCommand(X)`. This way, I'd be sure the two would refer to the same method.
Mehrdad Afshari
I agree with your thinking, but the equality still fails, unfortunately. I've edited my post so you can see the debugger output at the bottom of it.
Berryl
I'm unable to further investigate the issue as it's really hard to test something like that by just looking at a code snippet here. I believe delegate equality works if the target object and method reference is equal. Something else must be wrong.
Mehrdad Afshari