HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

GetCommonBaseClass: Readabiliy and Functionality

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andfunctionalitygetcommonbaseclassreadabiliy

Problem

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:

  • If one of the arguments is null, code A returns null and code B returns object.



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



My two questions for you:

  • Which code is more readable?



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

a. return null.

b. return object.

c. throw an exception?

Code A:

/// 
    /// Returns the most common type of two types.
    /// If no common type can be found, null is returned.
    /// 
    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;
        }
    }

    /// 
    /// Returns the most common type of one or more types.
    /// If no common type can be found, null is returned.
    /// 
    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:

```
/// Finds the most derived common base class of all the provided types, or System.Object if there is no common base class
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];

Solution

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.

Context

StackExchange Code Review Q#25628, answer score: 3

Revisions (0)

No revisions yet.