trjhtr

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.

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().
 

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...

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.

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);