Reusable implementation of IComparable<T>

Question

I often find myself implementing IComparable{T} in different classes for sorting in a very similar fashion:

1. Values meeting criteria X and Y should be sorted to the front
2. Values meeting criteria Z and W should be sorted to the back
3. Values not meeting any special criteria can stay where they are

So, I've created a class that abstracts this comparison style. It works okay, but I feel like there may be a code smell here.

Is there anything conceptually wrong with this class? Is there anything that could be simplified, or made more robust?

/// <summary>
/// Implementation of IComparer that uses a mutable collection of
/// delegates to compare instances.
/// </summary>
/// <remarks>Each condition is added with a boolean value indicating 
/// whether an instance meeting the condition should be considered 
/// "less than" or "greater than" instances not meeting the condition.
/// Conditions are evaluated in the order they are added to the collection.
/// If both instances have the same result for a condition, the condition has no effect.
/// If both instances have the same result for all conditions, the comparison result is 0.</remarks>
public class DynamicComparer<T> :
    IComparer<T>,
    IEnumerable<KeyValuePair<Func<T, bool>, bool>> {

    private readonly List<KeyValuePair<Func<T, bool>, bool>> conditions;
    private bool isReadOnly;

    public DynamicComparer() {
        this.conditions = new List<KeyValuePair<Func<T, bool>, bool>>();
    }

    #region Condition collection
    public bool IsReadOnly {
        get { return isReadOnly; }
        set {
            if (isReadOnly == true)
                throw new InvalidOperationException("Readonly");
            isReadOnly = value;
        }
    }

    public void Clear() {
        if (isReadOnly == true)
            throw new InvalidOperationException("Readonly"); 
        conditions.Clear();
    }

    public void Add(Func<T, bool> condition, bool prefer) {
        if (condition == null) throw new ArgumentNullException("condition");
        if (isReadOnly == true)
            throw new InvalidOperationException("Readonly");
        conditions.Add(new KeyValuePair<Func<T, bool>, bool>(condition, prefer));
    }

    public IEnumerator<KeyValuePair<Func<T, bool>, bool>> GetEnumerator() { return conditions.GetEnumerator(); }
    IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); }
    #endregion

    #region Comparison
    public int Compare(T x, T y) {
        foreach (var condition in conditions) {
            var result = condition.Value
                ? Prefer(condition.Key, x, y)
                : Defer(condition.Key, x, y);

            if (result != 0)
                return result;
        }

        return 0;
    }

    /// <summary>
    /// Tests both items. 
    /// If both tests have same result, returns 0; 
    /// otherwise, sorts passing item first.
    /// </summary>
    private int Prefer(Func<T, bool> condition, T x, T y) {
        var testX = condition(x);
        var testY = condition(y);

        return testX
            ? (testY ? 0 : -1)
            : (testY ? 1 : 0);
    }

    /// <summary>
    /// Tests both items. 
    /// If both tests have same result, returns 0; 
    /// otherwise, sorts failing item first.
    /// </summary>
    private int Defer(Func<T, bool> condition, T x, T y) {
        var testX = condition(x);
        var testY = condition(y);

        return testX
            ? (testY ? 0 : 1)
            : (testY ? -1 : 0);
    }
    #endregion
}

Here is a derived class:

public class SubReportComparer : DynamicComparer<ISubReport> {

    public SubReportComparer() : base() {
        Add(sub => sub == null, true);
        Add(sub => sub.Name == "FirstSubReport", true);
        Add(sub => sub.IsIssue, true);

        Add(sub => sub.Name == "LastSubReport", false);
        Add(sub => sub.Name == "2ndToLast", false);

        IsReadOnly = true;
    }
}

And here is what client code will look like:

List<ISubReport> subs = GetSubReports();
subs.Sort(new SubReportComparer());

Answer 1

• There is no need to test if a boolean is equals to true, unless it is a nullable boolean (null?).

• What is a KeyValuePair<Func<T, bool>, bool>? A function T to boolean, and a another boolean is really confusing, mostly the second part which is meant for sort direction. Don't be lazy. If you went ahead and created a dynamic comparer, why left out these two :

  public class SortCondition<T>
  {
      public SortDirection Direction { get; private set; }
      public Func<T, bool> Condition { get; private set; }
  
      public SortCondition(SortDirection direction, Func<T, bool> condition)
      {
          this.Direction = direction;
          this.Condition = condition;
      }
  }
  public enum SortDirection
  {
      Start = -1, End = 1
  }
  

• Prefer and Defer could be combine into a single method, or even fit into Compare.

  public int Compare(T x, T y)
  {
      foreach (var sortCondition in sortConditions)
      {
          var testX = sortCondition.Condition(x);
          var testY = sortCondition.Condition(y);
  
          if (testX == testY)
              continue;
  
          return testX ? +(int)sortCondition.Direction : -(int)sortCondition.Direction;
      }
  
      return 0;
  }
  

• Do align your bracket vertically, unless you have a good reason not to.

  ---

Full code :

void Main()
{
    var subs = new List<SubReport>
    {
        new SubReport { Name = "LastSubReport" },
        new SubReport { Name = "qwe" },
        new SubReport { Name = "asd" },
        new SubReport { Name = "FirstSubReport" },
        new SubReport { Name = "zxc", IsIssue = true },
        new SubReport { Name = "2ndToLast" },
    };

    subs.Sort(new SubReportComparer());
    subs.Dump();
}

// Define other methods and classes here
public class DynamicComparer<T> : IComparer<T>, IEnumerable<SortCondition<T>>
{
    private readonly List<SortCondition<T>> sortConditions;
    private bool isReadOnly;
    public bool IsReadOnly
    {
        get { return isReadOnly; }
        set
        {
            if (isReadOnly == true)
                throw new InvalidOperationException("Readonly");

            isReadOnly = value;
        }
    }

    public DynamicComparer()
    {
        sortConditions = new List<SortCondition<T>>();
    }
    public void Add(SortDirection direction, Func<T, bool> condition)
    {
        if (condition == null)
            throw new ArgumentNullException("Condition");
        if (isReadOnly == true)
            throw new InvalidOperationException("ReadOnly");

        sortConditions.Add(new SortCondition<T>(direction, condition));
    }
    /// <summary>
    /// Sorts passing item first.
    /// </summary>
    public void MoveToStart(Func<T, bool> condition)
    {
        Add(SortDirection.Start, condition);
    }
    /// <summary>
    /// Sorts failing item first.
    /// </summary>
    public void MoveToEnd(Func<T, bool> condition)
    {
        Add(SortDirection.End, condition);
    }

    public int Compare(T x, T y)
    {
        foreach (var sortCondition in sortConditions)
        {
            var testX = sortCondition.Condition(x);
            var testY = sortCondition.Condition(y);

            if (testX == testY)
                continue;

            return testX ? +(int)sortCondition.Direction : -(int)sortCondition.Direction;
        }

        return 0;
    }

    public IEnumerator<SortCondition<T>> GetEnumerator() { return sortConditions.GetEnumerator(); }
    IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); }
}
public class SortCondition<T>
{
    public SortDirection Direction { get; private set; }
    public Func<T, bool> Condition { get; private set; }

    public SortCondition(SortDirection direction, Func<T, bool> condition)
    {
        this.Direction = direction;
        this.Condition = condition;
    }
}
public enum SortDirection
{
    Start = -1, End = 1
}

public class SubReportComparer : DynamicComparer<ISubReport>
{
    public SubReportComparer() : base()
    {
        MoveToStart(sub => sub == null);
        MoveToStart(sub => sub.Name == "FirstSubReport");
        MoveToStart(sub => sub.IsIssue);

        MoveToEnd(sub => sub.Name == "LastSubReport");
        MoveToEnd(sub => sub.Name == "2ndToLast");

        IsReadOnly = true;
    }
}

public interface ISubReport
{
    string Name { get; }
    bool IsIssue { get; }
}
public class SubReport : ISubReport
{
    public string Name { get; set; }
    public bool IsIssue { get; set; }
}

Answer 2

Well, I do not actually think that the following is the shortest solution, but it is SOLID, so is maintainable, extendable, and reusable. First of all – how to use:

class SubReport
{
    public static readonly IComparer<SubReport> Comparer = 
        CompositeComparer<SubReport>.Empty
            .Prefer(sub => sub == null)
            .Prefer(sub => sub.Name == "FirstSubReport")
            .Prefer(sub => sub.IsIssue)
            .Defer(sub => sub.Name == "LastSubReport")
            .Defer(sub => sub.Name == "2ndToLast");

    public string Name { get; set; }
    public bool IsIssue { get; set; }
}

What we consume here are the following (classes are short, so there is probably no need to explain the code – I basically combine multiple implementations of IComparer<T>):

public static class ComparerComposition
{
    public static IComparer<T> Invert<T>(this IComparer<T> comparer) =>
        new InvertedComparer<T>(comparer);

    public static IComparer<T> ThenBy<T>(this IComparer<T> first, IComparer<T> second) =>
        new CompositeComparer<T>(first, second);

    public static IComparer<T> ThenByDescending<T>(this IComparer<T> first, IComparer<T> second) =>
        new CompositeComparer<T>(first, second.Invert());

    public static IComparer<T> Prefer<T>(this IComparer<T> first, Predicate<T> test) =>
        first.ThenBy(new TestComparer<T>(test));

    public static IComparer<T> Defer<T>(this IComparer<T> first, Predicate<T> test) =>
        first.ThenByDescending(new TestComparer<T>(test));
}

Swaps x and y, so result is inverted:

public class InvertedComparer<T> : IComparer<T>
{
    public InvertedComparer(IComparer<T> inner)
    {
        Inner = inner;
    }

    public int Compare(T x, T y) => 
        Inner.Compare(y, x);

    IComparer<T> Inner { get; }
}

Implements multilevel sorting:

public class CompositeComparer<T> : IComparer<T>
{
    public static readonly IComparer<T> Empty = new CompositeComparer<T>();

    public CompositeComparer(params IComparer<T>[] comparers)
    {
        Comparers = comparers;
    }

    public int Compare(T x, T y) =>
        Comparers
            .Select(c => c.Compare(x, y))
            .FirstOrDefault(r => r != 0);

    IComparer<T>[] Comparers { get; }
}

To sort by condition match:

public class TestComparer<T> : IComparer<T>
{
    public TestComparer(Predicate<T> test)
    {
        Test = test;
    }

    public int Compare(T x, T y)
    {
        var testX = Test(x);
        var testY = Test(y);
        return testX
            ? (testY ? 0 : -1)
            : (testY ? 1 : 0);
    }

    Predicate<T> Test { get; }
}

Please note that you can use this extension methods to compose comparers for other purposes, like multilevel multidirectional sorting. Hope it helps.

Answer 3

Your Defer method can return -Prefer. In fact you may wish to delete Defer based on this.

Instead of a KeyValuePair you might prefer to use Tuple instead, the only reason being that you don't really have any kind of structure key = value.

Instead of using IsReadOnly you might opt for putting your Add method protected and making DynamicComparer abstract so you always have to inherit from it. I would prefer to create an instance of DynamicComparer and I wouldn't care for IsReadOnly at all in that case though.

Answer 4

Additional to the answer from Bruno Costa that contains already some valid points

• Instead of using a KeyValuePair or Tuple, consider to use a custom type (maybe private class)
• I would also drop the ReadOnly flag. If you realy need it ,I would prefer to have a method Seal(), Freeze() or Lock() with a corresponding read-only property (IsSealed, IsFrozen, IsLocked) or something like that... Making a property one-time settable seems a little bit strange to me.
• Instead of having a boolean flag as parameter, I would provide 2 add methods AddPrefer and AddDefer. That is more readable because the boolean value is not descriptive.
• Why does the class provide access to the internal list of conditions by implementing the IEnumerable interface? Because I can not see any use case for that, I would drop it.

Answer 5

An alternative solution is to create a function to get take an item and get that item's sorting order and then use that in an OrderBy(). This method takes a collection of the tests for the front and a collection of the tests for the back and returns such a function.

public Func<T, int> GetFuncToGetOrder<T>(Func<T, bool>[] frontTests, Func<T, bool>[] backTests)
{
    Func<T, int> getOrder = x =>
        Enumerable.Range(0, frontTests.Count()).Where(i => frontTests[i](x)).Select(i => i - frontTests.Count())
        .Concat(Enumerable.Range(0, backTests.Count()).Reverse().Where(i => backTests[i](x)).Select(i => i + 1))
        .DefaultIfEmpty(0)
        .First();
    return getOrder;
}

The returned function takes an item and returns its sorting order, ranging from -frontTests.Count() to backTests.Count(), with 0 meaning it didn't match any tests.

You can call the method like this:

Func<int, ISubReport>[] frontTests =
{
    sub => sub == null,
    sub => sub.Name == "FirstSubReport",
    sub => sub.IsIssue
};
Func<int, ISubReport>[] backTests =
{        
    sub => sub.Name == "2ndToLast",
    sub => sub.Name == "LastSubReport"
};
Func<ISubReport, bool> getOrder = GetFuncToGetOrder<ISubReport>(frontTests, backTests);

This implementation is a little different in that the backTests are ordered in reverse. I think this makes it easier to see what the order of things will be, but if you want to keep the original ordering logic you can make the required adjustments to GetFuncToGetOrder() You can then use getOrder in the OrderBy() method:

List<ISubReport> subs = GetSubReports().OrderBy(getOrder).ToList();

If you still want to implement IComparable, you can use getOrder there as well:

public int Compare(T x, T y)
{
    return getOrder(x).CompareTo(getOrder(y));
}