GetCommonBaseClass: Readabiliy and Functionality

Question

I have two pieces of they which do almost the same thing. They both return the most common base class of two types. They differ in two aspect:

1. If one of the arguments is null, code A returns null and code B returns object.
2. If one of the arguments is an interface type, code A returns null and code B returns object.

My two questions for you:

1. Which code is more readable?

2. When it comes down to those differences, should the code:

   a. return null.
   b. return object.
   c. throw an exception?

Code A:

 /// <summary>
 /// Returns the most common type of two types.
 /// If no common type can be found, null is returned.
 /// </summary>
 static public Type GetCommonBaseClass(Type a, Type b)
 {
 if ((a == null) || (b ==null))
 return null;
 if (a.IsInterface || b.IsInterface)
 return null;
 if (a.IsAssignableFrom(b))
 return a;
 while (true)
 {
 if (b.IsAssignableFrom(a))
 return b;
 b = b.BaseType;
 }
 }
 /// <summary>
 /// Returns the most common type of one or more types.
 /// If no common type can be found, null is returned.
 /// </summary>
 static public Type GetCommonBaseClass(params Type[] types)
 {
 if ((types == null) || (types.Length == 0))
 return null;
 Type type = types[0];
 for (int i = 0; i < types.Length; i++)
 type = GetCommonBaseClass(type, types[i]);
 return type;
 }

Code B:

 /// <summary> Finds the most derived common base class of all the provided types, or System.Object if there is no common base class </summary>
 public static Type CommonBaseClass(params Type[] types)
 {
 if(ReferenceEquals(types,null)) return typeof(object);
 types = types.Where(x => !ReferenceEquals(x,null)).Distinct().ToArray();
 switch (types.Length)
 {
 case 0: return typeof(object);
 case 1: return types[0].IsInterface ? typeof(object): types[0];
 default:
 IEnumerable<IEnumerable<Type>> hierarchies = types.Select(ClassHierarchy).OrderBy(x => x.Count());
 Queue<Type> smallest = new Queue<Type>(hierarchies.First().Reverse());
 hierarchies = hierarchies.Skip(1);
 do
 {
 int maxPossible = smallest.Count;
 hierarchies = hierarchies.Select(each => each.Take(maxPossible));
 Type candidate = smallest.Dequeue();
 if (hierarchies.All(each => each.Last() == candidate))
 return candidate;
 } while (smallest.Count > 1);
 return typeof(object);
 }
 }
 ///<summary>Gets the class hierarchy of the provided type, in order of derivation, e.g. : (System.Object,CustomBaseType,CustomConcreteType,...), or the singleton of System.Object type if the provided type is an interface or null </summary>
 public static IEnumerable<Type> ClassHierarchy(this Type type)
 {
 if (type == null || type.IsInterface) type = typeof(object);
 var stack = new Stack<Type>();
 do
 {
 stack.Push(type);
 type = type.BaseType;
 } while (type != null);
 return stack;
 }

• dreza: I definitely find the first option more readable
• Bobson: object is the base for everything, so I don't know why you wouldn't want to return it if there's nothing more specific.
• PSGuy: I agree with @Bobson. In A, when if (a.IsInterface || b.IsInterface) is hit, you should be returning typeof(object) (or some other reference to the object type). As mentioned, it is the base of every object in the .NET Framework.

Answer 1

I think that code A is quite a lot simpler, so it's also more readable. It's quite clear what GetCommonBaseClass(Type a, Type b) is supposed to do and GetCommonBaseClass(params Type[] types) is also quite straightforward. On the other hand, CommonBaseClass(params Type[] types) in Code B is much more complicated, it's hard to see what's actually going on in all that code.

Also, in code A, GetCommonBaseClass(params Type[] types) could be simplified even more by using something like types.Aggregate(GetCommonBaseClass).

Regarding the edge cases, I think that, null inputs should cause an ArgumentNullException to be thrown. (Or not, if null is really a valid value.) With interfaces, I think the correct behavior depends on your requirements. If you don't know, I think the safest choice is to throw an exception. It's trivial to allow interfaces later on, if that's needed. On the other hand, adding a restriction could cause a issues with older code that uses your method.

• radarbob: +1. The only way returning object given null parameter(s) makes sense is if, by design, you never return object for 2 "objective" parameters. After all what class does not derive from object? ... I hope you have good documentation!

Answer 2

Which is more readable is quite a subjective question, however if you ask me I choose the first option. There's however something that bothers me more. You should never allow a potential exception to propagate in your code (this is returning null, you'll find yourself asking again if x is null, and what if you forget once???). You should handle this null value responsible or throw an ArgumentNullException(). A good approach as I see it could be returning object, since nearly everything in C# inherits from object, as you do in your second choice. You can read more here.

Answer 3

1. A is more readable obviously.
2. In case of interface type, the method should return null. In case of null parameter, the method should throw exception.