3
\$\begingroup\$

Below is the implementation of a comparer that compares objects of type Region first by index and then by it's type. The sort order of the type is defined within the comparer.

I know, it's a trivial piece of code, however I am happy about any comments about the realization. :)

public class Region
{
    public RegionType Type { get; set; }
    public int Index { get; set; }
}

public enum RegionType
{
    DeletionBeforeNamedRegion,
    DeletionWithinNamedRegion,
    DeletionAfterNamedRegion,
    InsertionBetweenNamedRegions,
    InsertionWithinNamedRegion,
    NamedRegion,
}

public class RegionComparer : IComparer<Region>
{
    private static readonly List<RegionType> sortOrderByType = new List<RegionType>
        {
            RegionType.DeletionBeforeNamedRegion,
            RegionType.NamedRegion,
            RegionType.DeletionWithinNamedRegion,
            RegionType.InsertionWithinNamedRegion,
            RegionType.DeletionAfterNamedRegion,
            RegionType.InsertionBetweenNamedRegions,
        };

    public int Compare(Region x, Region y)
    {
        if (x == null && y == null)
            return 0;
        if (x == null) return -1;
        if (y == null) return 1;

        var result = x.Index.CompareTo(y.Index);
        if (result != 0) return result;

        var xSort = sortOrderByType.IndexOf(x.Type);
        var ySort = sortOrderByType.IndexOf(y.Type);

#if DEBUG
        if (xSort < 0 || ySort < 0)
            throw new NotImplementedException($"Sort order for region type '{(xSort < 0 ? x.Type : y.Type)}' is not defined. Please extend the '{nameof(sortOrderByType)}' list." );
#else
        if (xSort < 0) xSort = int.MaxValue;
        if (ySort < 0) ySort = int.MaxValue;
#endif
        return xSort.CompareTo(ySort);
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ Why do you throw an exception in debug only? I'd say that should be an assertion instead, if it's just to increase visibility. \$\endgroup\$ Commented Jan 3, 2017 at 10:24
  • \$\begingroup\$ Good point... that is probably more readable. Thanks. \$\endgroup\$
    – JanDotNet
    Commented Jan 3, 2017 at 10:46
  • \$\begingroup\$ Is the enum standard this way of comparing or can the order of the enum change based on what is being compared? \$\endgroup\$ Commented Jan 3, 2017 at 15:17
  • \$\begingroup\$ You do know that enum CompareTo honors the order of the enum declaration? \$\endgroup\$
    – paparazzo
    Commented Jan 3, 2017 at 17:32
  • \$\begingroup\$ @Paparazzi: Yes, I am aware of that... actually, that was my first approach, but after further reflection, I decide that the enum is not right place for defining the order for the comparer. \$\endgroup\$
    – JanDotNet
    Commented Jan 3, 2017 at 17:38

1 Answer 1

2
\$\begingroup\$
private static readonly List<RegionType> sortOrderByType = new List<RegionType>
{
    RegionType.DeletionBeforeNamedRegion,
    RegionType.NamedRegion,
    RegionType.DeletionWithinNamedRegion,
    RegionType.InsertionWithinNamedRegion,
    RegionType.DeletionAfterNamedRegion,
    RegionType.InsertionBetweenNamedRegions,
};

Sorting needs to be fast, right? So how about using a dictionary for the oder lookup and not the O(n) List.IndexOf?

private static readonly Dictionary<RegionType, int> _regionTypeOrder = new RegionType[] 
{
    RegionType.DeletionBeforeNamedRegion,
    RegionType.NamedRegion,
    RegionType.DeletionWithinNamedRegion,
    RegionType.InsertionWithinNamedRegion,
    RegionType.DeletionAfterNamedRegion,
    RegionType.InsertionBetweenNamedRegions,
}
.Select((x, i) => new { Key = x, Order = i })
.ToDictionary(x => x.Key, x => x.Order);

with it, the lines

var xSort = sortOrderByType.IndexOf(x.Type);
var ySort = sortOrderByType.IndexOf(y.Type);

can become

var xSort = _regionTypeOrder[x.Type];
var ySort = _regionTypeOrder[y.Type];

var xSort = sortOrderByType.IndexOf(x.Type);
var ySort = sortOrderByType.IndexOf(y.Type);

if (xSort < 0) xSort = int.MaxValue;
if (ySort < 0) ySort = int.MaxValue;

the index should never be less then zero. If it is, then there is an error. Its an enum and the property isn't nullable so I think the protection isn't necessary.

After getting ther RegionType order from the dictionary you should be able to immediately call this

return xSort.CompareTo(ySort);
\$\endgroup\$
2
  • \$\begingroup\$ 1) Interesting.. I thought for few items (~less than 10), searching an item in a list is faster then a dictionary because it needs only few equality comparisons whereas the dictionary needs to calculate the hash value. However, after some performance tests, it seems that the dictionary is always faster or at least as fast as the list... :) \$\endgroup\$
    – JanDotNet
    Commented Jan 3, 2017 at 11:09
  • \$\begingroup\$ 2) It is possible, that someone adds a new value to the enum but doesn't change the comparer. Therefore, it make sense to catch that error and give a meaningful error message. However, it may be better to use Debug.Assert as Cody Gray suggested. \$\endgroup\$
    – JanDotNet
    Commented Jan 3, 2017 at 11:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.