Sorting based on multiple fields using IComparer

Question

I use the below code to sort List<DataAccessViewModel> list.

Here is the sort order :

1. PriorityScore
2. MName
3. CName
4. FName

It works as expected.

public int Compare(DataAccessViewModel x, DataAccessViewModel y)
{
    if (x == null || y == null)
    {
        return 0;
    }

    return x.CompareTo(y);
}

public int CompareTo(DataAccessViewModel mod)
{
    int retval = (int)(this.PriorityScore?.CompareTo(mod.PriorityScore));
    if(retval != 0)
        return retval;
    else
    {
        retval = (this.MName ?? "zzzzzzzzzzzzz").CompareTo(mod.MName ?? "zzzzzzzzzzzzz");
        if (retval != 0)
            return retval;
        else
        {
            retval = (this.CName ?? "zzzzzzzzzzzzz").CompareTo(this.CName ?? "zzzzzzzzzzzzz");
            if (retval != 0)
                return retval;
            else
                retval = (this.FName ?? "zzzzzzzzzzzzz").CompareTo(this.FName ?? "zzzzzzzzzzzzz");
        }
    }
        
    return retval;
}

But the code looks clunky to me. Is there any better way of doing it or is this it ?

Edit: here is the DataAccessViewModel with relevant properties

public class DataAccessViewModel : IComparer<DataAccessViewModel>
{
    public string CName { get; set; }
    public string MName { get; set; }
    public string FName { get; set; }
    public int? PriorityScore { get; set; }
}

Answer 1

First of all, your code has an error. Once you do this.PriorityScore?.CompareTo(mod.PriorityScore), for a PriorityScore that I assume it's a Nullable int, for any null value you will get null, then casting to int will fail. Or if mod.PriorityScore is null, the CompareTo method will fail.

Fixing that, if you return values from an if block, you don't really need an else, so your code could look like this:

int retval = (this.PriorityScore ?? 0).CompareTo(other.PriorityScore ?? 0);
if (retval != 0) return retval;
retval = (this.MName ?? "zzzzzzzzzzzzz").CompareTo(other.MName ?? "zzzzzzzzzzzzz");
if (retval != 0) return retval;
retval = (this.CName ?? "zzzzzzzzzzzzz").CompareTo(this.CName ?? "zzzzzzzzzzzzz");
if (retval != 0) return retval;
retval = (this.FName ?? "zzzzzzzzzzzzz").CompareTo(this.FName ?? "zzzzzzzzzzzzz");
return retval;

You could use Linq, although this would not be very efficient as performance goes, it would be more readable:

if (this.PriorityScore == other.PriorityScore
    && this.MName == other.MName
    && this.CName == other.CName
    && this.FName == other.FName) return 0;
return new[] { this, other }
    .OrderBy(vm => vm.PriorityScore)
    .ThenBy(vm => vm.MName)
    .ThenBy(vm => vm.CName)
    .ThenBy(vm => vm.FName)
    .First() == this ? -1 : 1;

(I removed the default values for readability, but you can add them) Also, if you can make DataAccessViewModel a struct, equality is baked in, so you can replace the first check with if (this.Equals(other)) return 0;. You can still do that if you override equality. This might not work well for the Compare method of an IComparable implementation, but it could work for your original List of DataAccessViewModel, if you want to move the ordering outside of the class:

var orderedList = list
    .OrderBy(vm => vm.PriorityScore)
    .ThenBy(vm => vm.MName)
    .ThenBy(vm => vm.CName)
    .ThenBy(vm => vm.FName)
    .ToList();

Another solution would be to implement a small method to simplify things:

public int CompareTo(DataAccessViewModel other)
{
    return compare(this.PriorityScore ?? 0, other.PriorityScore ?? 0)
        ?? compare(this.MName ?? "zzzzzzzzzzzzz", other.MName ?? "zzzzzzzzzzzzz")
        ?? compare(this.CName ?? "zzzzzzzzzzzzz", other.CName ?? "zzzzzzzzzzzzz")
        ?? compare(this.FName ?? "zzzzzzzzzzzzz", other.FName ?? "zzzzzzzzzzzzz")
        ?? 0;
}

private int? compare(IComparable o1,IComparable o2)
{
    var retval = o1.CompareTo(o2);
    return retval == 0 ? (int?)null : retval;
}

I believe this would be readable and performant enough.

Answer 2

Properties

For the sake of simplicity let's assume your view model's properties are defined like this:

public int? PriorityScore { get; set; }
public string MName { get; set; }
public string CName { get; set; }
public string FName { get; set; }

Compare

Your Compare method can be simply implemented with the following one-liner:

private const int SamePositionInOrdering = 0;
public int Compare(DataAccessViewModel lhs, DataAccessViewModel rhs)
    => lhs is object && rhs is object ? lhs.CompareTo(rhs) : SamePositionInOrdering;

• lhs is object is the new way in C# 9 to express lhs is not null
• I've used left and right hand side as parameter names, because I think they are more expressive than x and y
• You might also consider to make it static since it does not rely on instance members

CompareTo

Here I make a separation of the PriorityScore and the rest of the properties. The reasoning is simple in case of PriorityScore you don't have fallback value if it is null.

First lets define an array with property selectors to specify the ordering of MName, CName and FName:

private readonly Func<DataAccessViewModel, IComparable>[] propertySelectors;
       
public DataAccessViewModel()
{
    propertySelectors = new Func<DataAccessViewModel, IComparable>[] {
        vm => vm.MName,
        vm => vm.CName,
        vm => vm.FName,
    };
}

• If you unfamiliar with the property selector concept then please visit this SO topic
• I've used IComparable instead of string or object, since we only care about the CompareTo method
  • This concept is generic enough to support other property types as well, like int

Then let's see how can we implement the CompareTo:

private const string FallbackValue = "zzzzzzzzzzzzz";
public int CompareTo(DataAccessViewModel that)
{
    int positionInOrdering = (int)(PriorityScore?.CompareTo(that.PriorityScore));
    if (positionInOrdering != SamePositionInOrdering) return positionInOrdering;

    foreach (var selector in propertySelectors)
    {
        var lhs = selector(this) ?? FallbackValue;
        var rhs = selector(that) ?? FallbackValue;

        positionInOrdering = lhs.CompareTo(rhs);
        if (positionInOrdering != SamePositionInOrdering) return positionInOrdering;
    }

    return SamePositionInOrdering;
}

• I've renamed your mod parameter to that, since we are comparing this and that :)
• I've also renamed your retval to positionInOrdering since it better expresses the intent IMHO
• As I said I've treated the PriorityScore differently because there is no fallback value for that if its value is null
• I've introduced some constants so the implementation is free from hard coded values.

---

For the sake of completeness here is the entire implementation of DataAccessViewModel class

public class DataAccessViewModel: IComparable<DataAccessViewModel>
{
    public int? PriorityScore { get; set; }
    public string MName { get; set; }
    public string CName { get; set; }
    public string FName { get; set; }

    private const string FallbackValue = "zzzzzzzzzzzzz";
    private const int SamePositionInOrdering = 0;
    private readonly Func<DataAccessViewModel, IComparable>[] propertySelectors;
       
    public DataAccessViewModel()
    {
        propertySelectors = new Func<DataAccessViewModel, IComparable>[] {
            vm => vm.MName,
            vm => vm.CName,
            vm => vm.FName,
        };
    }
        
    public int Compare(DataAccessViewModel lhs, DataAccessViewModel rhs)
        => lhs is object && rhs is object ? lhs.CompareTo(rhs) : SamePositionInOrdering;

    public int CompareTo(DataAccessViewModel that)
    {
        int positionInOrdering = (int)(PriorityScore?.CompareTo(that.PriorityScore));
        if (positionInOrdering != SamePositionInOrdering) return positionInOrdering;

        foreach (var selector in propertySelectors)
        {
            var lhs = selector(this) ?? FallbackValue;
            var rhs = selector(that) ?? FallbackValue;

            positionInOrdering = lhs.CompareTo(rhs);
            if (positionInOrdering != SamePositionInOrdering) return positionInOrdering;
        }

        return SamePositionInOrdering;
    }
}

---

I've used the following data for testing:

List<DataAccessViewModel> models = new()
{
    new() { PriorityScore = 1, MName = "M", CName = "C", FName = "F" },
    new() { PriorityScore = 1, MName = "M", CName = "C", FName = "F" },
    new() { PriorityScore = 0, MName = "M", CName = "C", FName = "F" },
    new() { PriorityScore = 2, MName = "N", CName = "C", FName = "F" },
    new() { PriorityScore = 2, MName = "M", CName = "C", FName = "F" },
    new() { PriorityScore = 4, MName = "M", CName = "C", FName = "G" },
    new() { PriorityScore = 4, MName = "M", CName = "C", FName = "F" },
    new() { PriorityScore = 3, MName = "M", CName = "D", FName = "F" },
    new() { PriorityScore = 3, MName = "M", CName = "C", FName = "F" },
};

models.Sort();
foreach (var model in models)
{
    Console.WriteLine($"{{ PriorityScore = {model.PriorityScore}, MName = {model.MName}, CName = {model.CName}, FName = {model.FName} }}");
}

The output is the following:

{ PriorityScore = 0, MName = M, CName = C, FName = F }
{ PriorityScore = 1, MName = M, CName = C, FName = F }
{ PriorityScore = 1, MName = M, CName = C, FName = F }
{ PriorityScore = 2, MName = M, CName = C, FName = F }
{ PriorityScore = 2, MName = N, CName = C, FName = F }
{ PriorityScore = 3, MName = M, CName = C, FName = F }
{ PriorityScore = 3, MName = M, CName = D, FName = F }
{ PriorityScore = 4, MName = M, CName = C, FName = F }
{ PriorityScore = 4, MName = M, CName = C, FName = G }

Answer 3

CompareTo is an IComparable<T> implementation and Compare is IComparer<T> implementation. So, it would be better if you use the proper interfaces for that.

The default value zzzzzzzzzzzzz this should be a constant, either inside the model, or on a wider access class (if it's used on other classes).

so, the your class design would be something like :

public class DataAccessViewModel : IComparable<DataAccessViewModel>, IComparer<DataAccessViewModel>
{
    private const string DefaultStringValue = "zzzzzzzzzzzzz";

    public int? PriorityScore { get; set; }
    public string MName { get; set; }
    public string CName { get; set; }    
    public string FName { get; set; }
    

    // IComparer<DataAccessViewModel>
    public int Compare(DataAccessViewModel source, DataAccessViewModel target) { ... }
    
    // IComparable<DataAccessViewModel>
    public int CompareTo(DataAccessViewModel target) => Compare(this, target);
    
}

for the Compare method, there are multiple ways, here are two additional ways to do it :

using Dictionary :

public int Compare(DataAccessViewModel source, DataAccessViewModel target)
{
    if (source == null || target == null)
    {
        return 0;
    }
    
    int retval = (int)(source.PriorityScore?.CompareTo(target.PriorityScore));
    
    if(retval != 0) return retval;
    
    Dictionary<string, string> comparsion = new Dictionary<string, string>
    {
        { source.MName, target.MName },
        { source.CName, target.CName },
        { source.FName, target.FName }
    };

    foreach(var item in comparsion)
    {
        var leftOperhand  = item.Key ?? DefaultStringValue;
        var rightOperhand = item.Value ?? DefaultStringValue;
        
        retval = string.Compare(leftOperhand, rightOperhand, StringComparison.InvariantCultureIgnoreCase);
        
        if(retval != 0) return retval;
    }
    
    return retval;
}

Using private methods:

private int? InternalStringCompare(string leftOperhand, string rightOperhand)
{
    // Always use StringComparison when comparing strings
    int? result = string.Compare(leftOperhand ?? DefaultStringValue, rightOperhand ?? DefaultStringValue, StringComparison.InvariantCultureIgnoreCase);          
    return result != 0 ? result : null;
}

private int? InternalIntCompare(int? leftOperhand, int? rightOperhand)
{
    int? result = (leftOperhand ?? 0).CompareTo(rightOperhand ?? 0);           
    return result != 0 ? result : null;
}

public int Compare(DataAccessViewModel source, DataAccessViewModel target)
{
    return source != null  && target != null ?
           InternalIntCompare(source.PriorityScore, target.PriorityScore) 
        ?? InternalStringCompare(source.MName, target.MName) 
        ?? InternalStringCompare(source.CName, target.CName) 
        ?? InternalStringCompare(source.FName, target.FName) 
        ?? 0 : 0;
}

Answer 4

Get rid of all "if/else" complexity

{
  var retval = x.PriorityScore.CompareTo(y.PriorityScore);

  if(retval = 0) 
     retval = CompareMname(DataAccessViewModel x, DataAccessViewModel y);

  return retval;
}

public int CompareMname(DataAccessViewModel x, DataAccessViewModel y)
{
  var retval = x.MName.CompareTo(y.MName);

  if(retval = 0) 
     retail = CompareCname(DataAccessViewModel x, DataAccessViewModel y);

  return retval;
}

public int CompareCname(DataAccessViewModel x, DataAccessViewModel y)
{
  var retval = x.CName.CompareTo(y.CName);

  if(retval = 0) 
     retval = CompareFname(DataAccessViewModel x, DataAccessViewModel y);

  return retval;
}

// this must be the last called comparison
public int CompareFname(DataAccessViewModel x, DataAccessViewModel y)
{
  return x.CName.CompareTo(y.CName);
}