3
\$\begingroup\$

I am trying to compare two lists containing reference types (custom classes) element-wise, ignoring order. My question relates to the Equals method as shown below.

using System.Collections.Generic;
using System.Linq;

namespace Luxaudi.ViewModels
{
    /// <summary>
    /// Contains information from the Canvas class that is relevant to the user interface.
    /// </summary>
    internal sealed class CanvasViewModel : ViewModel
    {
        /// <summary>
        /// The display title of the canvas.
        /// </summary>
        public readonly string Title;
        /// <summary>
        /// The timeline view model belonging to the canvas view model.
        /// </summary>
        public readonly TimelineViewModel TimelineViewModel;
        /// <summary>
        /// Contains information from the Canvas class that is relevant to the user interface.
        /// </summary>
        public readonly List<NoteMapViewModel> NoteMapViewModels;
        /// <summary>
        /// Contains information from the Canvas class that is relevant to the user interface.
        /// </summary>
        public readonly List<ScaleViewModel> ScaleViewModels;

        /// <summary>
        /// 
        /// </summary>
        /// <param name="title">The display title of the canvas project.</param>
        /// <param name="timelineViewModel">The timeline view model which represents the Timeline of the Canvas.</param>
        public CanvasViewModel(string title, TimelineViewModel timelineViewModel)
        {
            Title = title;
            TimelineViewModel = timelineViewModel;
            ScaleViewModels = new List<ScaleViewModel>();
            NoteMapViewModels = new List<NoteMapViewModel>();
        }

        /// <summary>
        /// 
        /// </summary>
        /// <param name="title">The display title of the canvas project.</param>
        /// <param name="timelineViewModel">The timeline view model which represents the Timeline of the Canvas.</param>
        /// <param name="scaleViewModels">The collection of scale view models belonging to the canvas view model.</param>
        /// <param name="noteMapViewModels">The collection of note map view models belonging to the canvas view model.</param>
        public CanvasViewModel(string title, TimelineViewModel timelineViewModel, List<ScaleViewModel> scaleViewModels, 
                               List<NoteMapViewModel> noteMapViewModels)
        {
            Title = title;
            TimelineViewModel = timelineViewModel;
            ScaleViewModels = scaleViewModels;
            NoteMapViewModels = noteMapViewModels;
        }

        /// <summary>
        /// Compare two CanvasViewModels for object equality.
        /// </summary>
        /// <param name="obj">The object to check for equality with.</param>
        /// <returns>False if the object is not a CanvasViewModel or the member attributes are not equal.</returns>
        public override bool Equals(object obj)
        {
            CanvasViewModel canvasViewModel = obj as CanvasViewModel;
            if(canvasViewModel != null)
            {
                if (canvasViewModel.Title == Title && canvasViewModel.TimelineViewModel == TimelineViewModel &&
                    canvasViewModel.ScaleViewModels.All(ScaleViewModels.Contains) && 
                    canvasViewModel.ScaleViewModels.Count == ScaleViewModels.Count &&
                    canvasViewModel.NoteMapViewModels.All(NoteMapViewModels.Contains) &&
                    canvasViewModel.NoteMapViewModels.Count == NoteMapViewModels.Count)
                    return true;
            }

            return false;
        }
    }
}

The CanvasViewModel inherits from the generic ViewModel class:

namespace Luxaudi.ViewModels
{
    /// <summary>
    /// The basic template of all view models. Contains information from domain classes that is relevant to the user interface.
    /// </summary>
    internal abstract class ViewModel
    {
        public override abstract bool Equals(object obj);
    }
}

Any gotchas to look out for here?

EDIT: Full code provided as recommended.

\$\endgroup\$
3
\$\begingroup\$

It looks OK to me, but you could optimize a little bit by comparing the "low-hanging fruits" first:

public override bool Equals(object obj)
{
  CanvasViewModel other = obj as CanvasViewModel;

  return ReferenceEquals(this, other) // other == this is "unsafe" if == is implemented by calling Equals().
    || other != null
    && other.ScaleViewModels.Count == ScaleViewModels.Count
    && other.NoteMapViewModels.Count == NoteMapViewModels.Count
    && other.Title == Title
    && other.TimelineViewModel == TimelineViewModel
    && other.ScaleViewModels.All(ScaleViewModels.Contains)
    && other.NoteMapViewModels.All(NoteMapViewModels.Contains);
}

Here it is first checked if the argument (reference) equals this. If it does, no further comparison is necessary. Further the scalars ScaleViewModels.Count and NoteMapViewModels.Count are fairly cheap to compare - compared to other.ScaleViewModels.All(...) and if they are different there is no need for further comparison etc...

\$\endgroup\$
  • \$\begingroup\$ Would adding parentheses to the && conditions make a difference in the ordering of the boolean evaluation? Is adding them even necessary? \$\endgroup\$ – Alluring Topaz Jun 7 '18 at 19:17
  • 1
    \$\begingroup\$ @AlluringTopaz: No, parentheses are only useful if you want to break the precedence between && and ||. Take a look at the documentation for these operators in C# \$\endgroup\$ – Henrik Hansen Jun 7 '18 at 19:51
  • \$\begingroup\$ The count is probably longer than the two below. \$\endgroup\$ – paparazzo Jun 14 '18 at 15:02
  • \$\begingroup\$ Nah, rolledback, I guess I broke something :-] \$\endgroup\$ – t3chb0t Jun 14 '18 at 18:35
  • 1
    \$\begingroup\$ @paparazzo: Well, we don't know about TimelineViewModel, but take a look at the source code for List<T>.Count and string.==. \$\endgroup\$ – Henrik Hansen Jun 15 '18 at 5:01
2
\$\begingroup\$
public readonly string Title;

You shouldn't be using public fields. That's what we have properties for. You can make them readonly too by specifying just the get method like that:

public string Title { get; }

I find it's much easier and less error-prone to implement the equality via the IEquatable<T> interface. This gives you a strongly typed Equals method that you can use to redirect from the default one.

The signagure of you class would become

internal sealed class CanvasViewModel : ViewModel, IEquatable<CanvasViewModel>

and the respective implementations will be

public override bool Equals(object obj)
{
    return obj is CanvasViewModel model && Equals(model);
}

You can save a few characters by using the new is operator that will try to cast the obj to the specified type and assign the result to the variable if it succeeds. Then it calls the second Equals method.

public bool Equals(CanvasViewModel other)
{
    if (ReferenceEquals(other, null)) return false;
    if (ReferenceEquals(other, this)) return true;

    return
        other.Title == Title && 
        other.TimelineViewModel == TimelineViewModel &&
        other.ScaleViewModels.All(ScaleViewModels.Contains) &&
        other.ScaleViewModels.Count == ScaleViewModels.Count &&
        other.NoteMapViewModels.All(NoteMapViewModels.Contains) &&
        other.NoteMapViewModels.Count == NoteMapViewModels.Count);
    }
}

The first two conditions here are pretty generic and you use them for pretty every implementation of Equals. It doesn't make sense to try to chain them with the actual comparison. Leave them alone and focus on the other properties that are now also much easier to handle when you don't have to care about even more () or tricky || and && combinations.


internal abstract class ViewModel
{
    public override abstract bool Equals(object obj);
}

We don't use abstract classes like this one because it has no default implementation for anything. It could, should be an interface. It's also not necessary to even define it because the Equals method can be overriden without it. It's already virtual because every object inherits it by default.

It would make much more sense if you defined it this way

internal interface IViewModel<T> : IEquatable<T>
{

}

and the respective class

internal sealed class CanvasViewModel : IViewModel<CanvasViewModel>
{
    ...
}

Some people argue that you shouldn't be using marker-interfaces, but I find they are very usefull and I do it all the time for various reasons.

\$\endgroup\$
  • \$\begingroup\$ I definitely will be updating the appropriate classes to match your advice. \$\endgroup\$ – Alluring Topaz Jun 15 '18 at 14:48
  • 1
    \$\begingroup\$ @AlluringTopaz you can do both... I often do. I start with an interface and when I need a default implementation for something I just add an abstract class inbetween and inherit other classes from it. From the interface point of view nothing has changed and this still remains your contract. \$\endgroup\$ – t3chb0t Jun 15 '18 at 14:51
  • 1
    \$\begingroup\$ Thanks :) I actually edited my comment thinking it would make sense to do exactly what you just said...then you posted lol. I will keep this in mind as well. \$\endgroup\$ – Alluring Topaz Jun 15 '18 at 14:54
1
\$\begingroup\$

As per Henrik Hansen's comment under the original post:

@AlluringTopaz: I think, the right way is to make a new answer, if you want to add to the original.

and based on the recommendation to use a strongly typed Equals method in t3chb0t's answer, I should clarify that since no explicit == operator was provided for TimelineViewModel then, the implementation of IEquatable.Equals in CanvasViewModel should be:

public bool Equals(CanvasViewModel other)
{
    if (ReferenceEquals(other, null)) return false;
    if (ReferenceEquals(other, this)) return true;

    return
        other.Title == Title && 
        other.TimelineViewModel.Equals(TimelineViewModel) &&
        other.ScaleViewModels.All(ScaleViewModels.Contains) &&
        other.ScaleViewModels.Count == ScaleViewModels.Count &&
        other.NoteMapViewModels.All(NoteMapViewModels.Contains) &&
        other.NoteMapViewModels.Count == NoteMapViewModels.Count);
}
\$\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.