Function to compare objects based on type enums

Question

I'm looking for a better way to implement a huge CompareTo function that relies on an Enum to perform sorting. The data structure is essentially a flattened Multi Level Group. Any ideas and concept to make this function easier to understand and extend are appreciated.

With the exception of of the Field 1,2,3 names everything is exactly as in my code.

public int CompareTo(object obj)
{
 var y = (ListEntry)obj;
 var result = string.CompareOrdinal(Field1, y.Field1);
 if (result != 0) { return result; }
 switch (Typ)
 {
 case TypEnum.None:
 switch (y.Typ)
 {
 case TypEnum.None:
 return string.CompareOrdinal(Field2, y.Field2);
 case TypEnum.MessageField2Sum:
 case TypEnum.MessageField2:
 case TypEnum.MessageField2Caption:
 case TypEnum.Field1:
 return -1;
 case TypEnum.Field3:
 case TypEnum.Field2Group:
 case TypEnum.SubGroup:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 return -1;
 case TypEnum.Field1Caption:
 return 1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 case TypEnum.Field1Caption:
 return -1;
 case TypEnum.Field1:
 return 1;
 case TypEnum.Field3:
 switch (y.Typ)
 {
 case TypEnum.None:
 case TypEnum.Field2Group:
 case TypEnum.Field3:
 case TypEnum.Field1Caption:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 Debug.Assert(y.Typ != TypEnum.Field3);
 return 1;
 case TypEnum.SubGroup:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 Debug.Assert(y.Typ != TypEnum.Field3);
 return -1;
 case TypEnum.Field1:
 case TypEnum.MessageField2Sum:
 case TypEnum.MessageField2:
 case TypEnum.MessageField2Caption:
 return -1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 case TypEnum.Field2Group:
 switch (y.Typ)
 {
 case TypEnum.None:
 case TypEnum.Field2Group:
 case TypEnum.Field1Caption:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 Debug.Assert(y.Typ != TypEnum.Field2Group);
 return 1;
 case TypEnum.Field3:
 case TypEnum.SubGroup:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 return -1;
 case TypEnum.MessageField2Sum:
 case TypEnum.MessageField2Caption:
 case TypEnum.MessageField2:
 case TypEnum.Field1:
 return -1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 case TypEnum.SubGroup:
 switch (y.Typ)
 {
 case TypEnum.None:
 case TypEnum.Field2Group:
 case TypEnum.Field3:
 case TypEnum.Field1Caption:
 case TypEnum.SubGroup:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 Debug.Assert(y.Typ != TypEnum.SubGroup);
 return 1;
 case TypEnum.MessageField2Sum:
 case TypEnum.MessageField2:
 case TypEnum.MessageField2Caption:
 case TypEnum.Field1:
 return -1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 case TypEnum.MessageField2Caption:
 switch (y.Typ)
 {
 case TypEnum.None:
 case TypEnum.Field2Group:
 case TypEnum.Field3:
 case TypEnum.Field1Caption:
 case TypEnum.SubGroup:
 return 1;
 case TypEnum.MessageField2:
 case TypEnum.MessageField2Sum:
 case TypEnum.Field1:
 return -1;
 case TypEnum.MessageField2Caption:
 default:
 throw new ArgumentOutOfRangeException();
 }
 case TypEnum.MessageField2Sum:
 switch (y.Typ)
 {
 case TypEnum.None:
 case TypEnum.Field2Group:
 case TypEnum.Field3:
 case TypEnum.Field1Caption:
 case TypEnum.SubGroup:
 case TypEnum.MessageField2Caption:
 return 1;
 case TypEnum.MessageField2:
 result = string.CompareOrdinal(Field2, GetField2Group(y.Field2));
 if (result != 0)
 {
 return result;
 }
 return 1;
 case TypEnum.MessageField2Sum:
 result = string.CompareOrdinal(Field2, y.Field2);
 if (result != 0)
 {
 return result;
 }
 throw new ArgumentOutOfRangeException();
 case TypEnum.Field1:
 return -1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 case TypEnum.MessageField2:
 switch (y.Typ)
 {
 case TypEnum.None:
 case TypEnum.Field2Group:
 case TypEnum.Field3:
 case TypEnum.Field1Caption:
 case TypEnum.SubGroup:
 case TypEnum.MessageField2Caption:
 return 1;
 case TypEnum.MessageField2:
 result = string.CompareOrdinal(GetField2Group(Field2), GetField2Group(y.Field2));
 if (result != 0)
 {
 return result;
 }
 result = string.CompareOrdinal(Field2, y.Field2);
 if (result != 0)
 {
 return result;
 }
 throw new ArgumentOutOfRangeException();
 case TypEnum.MessageField2Sum:
 result = string.CompareOrdinal(GetField2Group(Field2), y.Field2);
 if (result != 0)
 {
 return result;
 }
 return -1;
 case TypEnum.Field1:
 return -1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 default:
 throw new ArgumentOutOfRangeException();
 }
 throw new NotImplementedException();
}

• 200_success: MyObject, MyEnum, Field1, etc. look like made-up identifiers. Please post real code — or at least code that realistically captures your intent — so that we can advise you properly.
• Florian: @200_success sry, I updated the question. The Field1,2,3 names are domain specific and won't help in the context of the question.

Answer 1

Your outer switch statement has 9 cases (and one default). If you take advantage of polymorphism, you can split this out into 9 classes with no need for some default handler.

Here's some information about switch statements being a "code smell":

• https://sourcemaking.com/refactoring/smells/switch-statements
• http://c2.com/cgi/wiki?SwitchStatementsSmell
• https://stackoverflow.com/questions/4417070/switch-statements-are-bad

Here's what you'd do with your code. First make your class abstract and give it a CompareTo method that calls child class CompareToField1Equal if the Field1s are equal:

abstract class MyClass {
 public int CompareTo(object obj) {
 var y = (ListEntry)obj;
 var result = string.CompareOrdinal(Field1, y.Field1);
 if (result != 0) { return result; }
 return this.CompareToField1Equal(y);
 }
 protected abstract int CompareToField1Equal(ListEntry y);
}

Every class that extends this MyClass will represent a case in your outer switch statement. These child classes must implement CompareToField1Equal. Here's a simple example that replaces the second case in your outer switch statement:

class Field1Caption : MyClass {
 protected override int CompareToField1Equal(ListEntry y) {
 return -1;
 }
}

That's it. Some of the cases in your outer switch statement have their own switches. Here's an example that replaces your first case:

class None : MyClass {
 protected override int CompareToField1Equal(ListEntry y) {
 case TypEnum.None:
 return string.CompareOrdinal(Field2, y.Field2);
 case TypEnum.MessageField2Sum:
 case TypEnum.MessageField2:
 case TypEnum.MessageField2Caption:
 case TypEnum.Field1:
 return -1;
 case TypEnum.Field3:
 case TypEnum.Field2Group:
 case TypEnum.SubGroup:
 result = string.CompareOrdinal(Field3, y.Field3);
 if (result != 0)
 {
 return result;
 }
 return -1;
 case TypEnum.Field1Caption:
 return 1;
 default:
 throw new ArgumentOutOfRangeException();
 }
}

Because your outer switch statement has 9 cases (and one default), you'll have 9 classes that extend MyClass. Note that you don't need a class that handles the default. This is an advantage of using polymorphism in this way. In what situation would your outer switch's default get hit? If you added a FieldX to your TypEnum and your Typ was equal to FieldX, you'd get to the default with its lovely ArgumentOutOfRangeException.

Now try to replicate this situation using my solution. You can't. The only way to have a FieldX "case" would be to have a FieldX class that extends MyClass, but since the CompareToField1Equal method is marked abstract, your code won't compile until you implement FieldX's CompareToField1Equal. This effectively turns a runtime exception into a compiler error. This is a good thing.

---

My class None above has a switch statement with 9 cases in it. You can use polymorphism in the same way to replace this switch statement with an abstract class and 9 child classes. This will be a rewrite of the None class. Again, the first step is to make None abstract ...:

abstract class None : MyClass {
 protected sealed override int CompareToField1Equal(ListEntry y) {
 return this.CompareToField1EqualByType(y);
 }
 protected abstract int CompareToField1EqualByType(ListEntry y);
}

Here's a child class that replaces the first case:

class NoneNone : None {
 protected override int CompareToField1EqualByType(ListEntry y) {
 return string.CompareOrdinal(Field2, y.Field2);
 }
}

Note that I marked the None.CompareToField1Equal method as sealed. This is not necessary, but I don't want grandchildren of MyClass overriding CompareToField1Equal. That'd be confusing.

---

Here's how you'd use this code:

MyClass left = new NoneNone();
MyClass right = new Field1Caption();
int result = left.CompareTo(right);

• Florian: I like that approach, but I'm not happy about naming. 'CompareToField1Equal' feels weird and "None" as a class name would be confusing as there is a (more or less unrelated) class NONE. I'm thinking about MyClassNone, but I'm not sure it would fit.

Answer 2

Encapsulating the comparer logic of each main switch in a separate class is definitely the way to go but I'd like to show you a slightly different approach.

Step 1

I suggest you additionaly implement the IComparer<T> interface. The reason for this is that it is generic and you don't have to cast anything inside it and it takes two arguments so it's easier to distinguish them because you need to prefix their members with the parameter name. The CompareTo method can call this interface.

public class ListEntry : IComparable, IComparer<ListEntry>
{
 public TypEnum Typ { get; }
 public string Field1 { get; }
 public string Field2 { get; }
 public string Field3 { get; }
 public string Field4 { get; }
 public int Compare(ListEntry x, ListEntry y)
 {
 // ... see step 4
 }
 public int CompareTo(object obj)
 {
 return Compare(this, (ListEntry)obj);
 }
}

Step 2

For each main switch create a class that implements the IComparer<T> interface. Here's an example for two of them:

public class NoneComparer : IComparer<ListEntry>
{
 public int Compare(ListEntry x, ListEntry y)
 {
 switch (y.Typ)
 {
 case TypEnum.None:
 return string.CompareOrdinal(x.Field2, y.Field2);
 case TypEnum.MessageField2Sum:
 case TypEnum.MessageField2:
 case TypEnum.MessageField2Caption:
 case TypEnum.Field1:
 return -1;
 case TypEnum.Field3:
 case TypEnum.Field2Group:
 case TypEnum.SubGroup:
 var field3Result = string.CompareOrdinal(x.Field3, y.Field3);
 if (field3Result != 0)
 {
 return field3Result;
 }
 return -1;
 case TypEnum.Field1Caption:
 return 1;
 default:
 throw new ArgumentOutOfRangeException();
 }
 }
}
public class Field1CaptionComparer : IComparer<ListEntry>
{
 public int Compare(ListEntry x, ListEntry y)
 {
 return -1;
 }
}

Step 3

Create a ComparerAttribute

[AttributeUsage(AttributeTargets.Field)]
class ComparerAttribute : Attribute
{
 public ComparerAttribute(Type comparerType)
 {
 // create the specified comparer
 Comparer = (IComparer<ListEntry>)Activator.CreateInstance(comparerType, null);
 }
 public IComparer<ListEntry> Comparer { get; }
}

and decorate each TypEnum value with it:

public enum TypEnum
{
 [Comparer(typeof(NoneComparer))]
 None,
 [Comparer(typeof(Field1CaptionComparer))]
 Field1Caption,
 MessageField2Sum,
 MessageField2,
 MessageField2Caption,
 Field1,
 Field3,
 Field2Group,
 SubGroup
}

Step 4

Implement the Compare method of the ListEntry so that it dynamicaly gets the comparer for the Typ property:

public int Compare(ListEntry x, ListEntry y)
{
 var result = string.CompareOrdinal(x.Field1, y.Field1);
 if (result != 0) { return result; }
 // get the comparer dynamicaly
 var typEnumField = typeof(TypEnum).GetMember(x.Typ.ToString()).First();
 var comparerAttribute = typEnumField.GetCustomAttribute(typeof(ComparerAttribute)) as ComparerAttribute;
 if (comparerAttribute == null) throw new NotSupportedException();
 // compare both list entries
 return comparerAttribute.Comparer.Compare(x, y);
}