TestExplorer 2.0 - From Grid to TreeView

Rubberduck's unit testing feature (heck, the whole project) started with a VBA unit testing framework that was ported from vba to c#; the VBA logic was pretty much simply rewritten in another language, and a dockable toolwindow is now available in the VBA IDE for Rubberduck users around the world:

For the next major release, the Test Explorer is getting a facelift. What was a WinForms user control in 1.x is going to be a WPF user control in 2.x - in fact as much as possible of the UI is going to be redone in WPF/XAML, and hosted in a WinForms/WPF control host.

Here's the markup for the Rubberduck 2.0 Test Explorer:

`

TestMethod

The Result property (and IsEditing property)

public TestResult Result
{
    get { return _result; } 
    set { _result = value; OnPropertyChanged();}
}

seems only to be set by the TestMethod object, so there is no need to have a public setter there. Making the setter either private or protected will increase its encapsulation.

The usage of OnPropertyChanged() should only be done if the property really changed, so changing it to

public TestResult Result
{
    get { return _result; } 
    private set 
    { 
        if(_result == value) { return; }
        _result = value; 
        OnPropertyChanged();
    }
}

will remove unneeded calls of said event.

private TestResult EvaluateResults()
{
    var result = TestResult.Success();

    if (_assertResults.Any(assertion => assertion.Outcome == TestOutcome.Failed || assertion.Outcome == TestOutcome.Inconclusive))
    {
        result = _assertResults.First(assertion => assertion.Outcome == TestOutcome.Failed || assertion.Outcome == TestOutcome.Inconclusive);
    }

    return result;
}

Here you are first checking with Any() if the wanted item exists and later you are calling First() to retrieve the first item.

A much easier way would be to just use FirstOrDefault() and using the null coalescing operator to return either the found item or TestResult.Success() like so

private TestResult EvaluateResults()
{
    var result = _assertResults.FirstOrDefault(assertion => assertion.Outcome == TestOutcome.Failed || assertion.Outcome == TestOutcome.Inconclusive);

    return result ?? TestResult.Success();
}

public override bool Equals(object obj)
{
    return obj is TestMethod
        && ((TestMethod)obj).QualifiedMemberName.Equals(QualifiedMemberName);
}

I don't like the usage of is together with a cast because it is just doing the cast two times. A better way would be to use as together with a null check like so

public override bool Equals(object obj)
{
    var method = obj as TestMethod;
    if (method == null) { return false; }

    return method.QualifiedMemberName.Equals(QualifiedMemberName);
}

public void CancelEdit()
{
    if (_cachedResult != null)
    {
        Result = _cachedResult;
    }

    _cachedResult = null;
    IsEditing = false;
}

here the setting of _cachedResult to null should be done inside the if. There is no need to set an object which is null to null.

StackExchange Code Review Q#102276, answer score: 7