patterncsharpMinor
GetCommonBaseClass: Readabiliy and Functionality
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:
My two questions for you:
-
When it comes down to those differences, should the code:
a. return
b. return
c. throw an exception?
Code A:
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];
- If one of the arguments is
null, code A returnsnulland code B returnsobject.
- If one of the arguments is an interface type, code A returns
nulland code B returnsobject.
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
On the other hand,
Also, in code A,
Regarding the edge cases, I think that,
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.