14
\$\begingroup\$

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());
\$\endgroup\$
  • \$\begingroup\$ Did you test the logic? Because I am not sure if the remaining items stay where they were.... \$\endgroup\$ – JanDotNet May 25 '16 at 18:55
  • \$\begingroup\$ "Values meeting criteria X and Y should be sorted to the front" Do you mean X or Y? \$\endgroup\$ – Jonathon Reinhart May 26 '16 at 16:45
  • \$\begingroup\$ @JonathonReinhart - Sorry for the ambiguity. I really want a sort of short-circuit OR behavior. Each predicate is evaluated against both inputs until either a predicate returns different results for each input, or the list of predicates is exhausted. In common English, it's basically a list of tie-breakers. \$\endgroup\$ – JamesFaix May 26 '16 at 23:51
11
\$\begingroup\$
  • 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; }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I am not sure if SortDirection/Ascending/Descending is the right wording because the value determines if the items are sorted to the start or to the end of the list. \$\endgroup\$ – JanDotNet May 25 '16 at 18:59
  • \$\begingroup\$ @JNS True, I had troubled naming them, so I borrowed a couple of words from linq. Looking back now, it doesn't seem to fit well. What do you think of SortDirection { Start, End } and Add[Prefer/Defer]? \$\endgroup\$ – Xiaoy312 May 25 '16 at 19:34
  • 1
    \$\begingroup\$ SortDirection { Start, End } sounds better.. However, AddRefer/Defer is tricky because the 'Add' part means the delegate that is added and the 'Prefer/Defer' part means the item that will be sorted. Maybe MoveUp/Down, MoveToStart/ToEnd or something like that? Because that is what the comparer does with the items defined by the delegate. \$\endgroup\$ – JanDotNet May 25 '16 at 20:08
9
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Elegant! I really like the fluent API style for that kind of object creation. \$\endgroup\$ – JanDotNet May 27 '16 at 5:58
  • \$\begingroup\$ @JNS Thanks. I wish Microsoft kept Primary Constructors in C# 6. They make a lot of sense, especially by having primary ctor parameters being available everywhere in a class - it could reduce amount of code lines by 80% for this type of coding... \$\endgroup\$ – Dmitry Nogin May 27 '16 at 7:32
  • 1
    \$\begingroup\$ This is nice indeed. I'll borrow from this solution too ;-) \$\endgroup\$ – t3chb0t Jun 23 '16 at 8:57
8
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ While they do not have a key-value relationship, neither Item1+Item2 nor Key+Value are preferred here. Both are just the programmer being lazy. \$\endgroup\$ – Xiaoy312 May 25 '16 at 18:53
  • \$\begingroup\$ @Xiaoy312 The programmer being lazy does not always imply that the code is not acceptable. I would use/see/mantain Tuple gladly in a place like this. Unfortunately I know that many others frown on it. \$\endgroup\$ – Bruno Costa May 25 '16 at 21:52
7
\$\begingroup\$

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.
\$\endgroup\$
  • 1
    \$\begingroup\$ GetEnumerator and the awkward Add are there to take advantage of C#'s collection initializer syntax. But I agree that being able to externally iterate the list of delegates is messy. \$\endgroup\$ – JamesFaix May 26 '16 at 3:15
0
\$\begingroup\$

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));
}
\$\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.