Smaller methods vs. Clear recursion method

Question

I'm reading Robert Martin's book "Clean Code" and most of what I've read makes sense and I'm trying to apply as much as I possibly can. One of the simplest most basic things he talks about is that methods should be small, he goes all the way as to say that they should not contain more than three lines.

Considering that I broke down a recursive method into two smaller ones but in a code review one of my peers suggested that it looked weird and that recursion was kind of hidden.

So, what is your opinion on that? I'm including both approaches below.

This is the one method approach.

private object GetObject(string propertyName, object dataObject)
{
  object returnObject;

  if (dataObject == null) return null;

  if (propertyName.Contains("."))
  {
    Type dataObjectType = dataObject.GetType();
    PropertyInfo dataObjectProperty = dataObjectType.GetProperty(propertyName.Substring(0, propertyName.IndexOf(".")));

    ThrowExceptionIfObjectOrPropertyInexistant(dataObjectProperty, dataObjectType.FullName, propertyName);

    object nestedObject = dataObjectProperty.GetValue(dataObject, null);
    string nestedObjectPropertyName = propertyName.Substring(propertyName.IndexOf(".") + 1);

    returnObject = GetObject(nestedObjectPropertyName, nestedObject);
  }
  else
  {
    returnObject = dataObject;
  }

  return returnObject;
}

This is the two methods one.

private object GetObject(string propertyName, object dataObject)
{
  object returnObject;

  if (dataObject == null) return null;

  if (propertyName.Contains("."))
  {
    returnObject = GetNestedObject(propertyName, dataObject);
  }
  else
  {
    returnObject = dataObject;
  }

  return returnObject;
}

private object GetNestedObject(string propertyName, object dataObject)
{
  object returnObject;
  Type dataObjectType = dataObject.GetType();
  PropertyInfo dataObjectProperty = dataObjectType.GetProperty(propertyName.Substring(0, propertyName.IndexOf(".")));

  ThrowExceptionIfObjectOrPropertyInexistant(dataObjectProperty, dataObjectType.FullName, propertyName);

  object nestedObject = dataObjectProperty.GetValue(dataObject, null);
  string nestedObjectPropertyName = propertyName.Substring(propertyName.IndexOf(".") + 1);

  returnObject = GetObject(nestedObjectPropertyName, nestedObject);
  return returnObject;
}

Answer 1

Since somebody gave me a minus one I guess I need to elaborate. Here's what I meant:

Most people here seem to be of opinion that in this case one big function is better then several small ones. I disagree. I think your original function is a monster that has at least 3 responsibilities.

First, you should turn recursion into a loop which is always a good thing. Second, you should apply the single responsibility principle and refactor your code as follows:

    private object GetInnermostObject(object dataObject, string propertyName){
     object currentObject = dataObject;
     foreach(propName in propertyName.Split(".")) currentObject = GetNestedObject(currentObject, propName);
     return currentObject;
    }

    private object GetNestedObject(object dataObject, string propertyName)
    {
     PropertyInfo property = GetDataObjectProperty(dataObject.GetType(), propertyName);
     object nestedObject = property.GetValue(dataObject, null);
     return new DataObjectWrapper(nestedObject);
    }

    private static PropertyInfo GetDataObjectProperty(Type dataObjectType, string propertyName){
     PropertyInfo dataObjectProperty = dataObjectType.GetProperty(propertyName);
     ThrowExceptionIfObjectOrPropertyInexistant(dataObjectProperty, dataObjectType.FullName, propertyName);
     return dataObjectProperty;
    }

You can improve this further (like I suggested originally) by making this code into a separate class that would encapsulate access to dataObject altogether, but this is beyond the topic of this question.

Answer 2

Your methods should be as short as possible but no shorter. In this case, all the inner function is doing is hiding the recursion. This is, as your collegue mentioned, unclear, and even misleading, since it means that the recursion proceeds

1 GetObject
2 GetNested Object
3 GetObject
4 GetNestedObject
....

It also means that the second version of the code will defeat pretty much any attempt by the compiler optimize the recursion into a loop.

Your first method fits into one screen; I think that's short enough, and the other choice is a bit confusing.

Answer 3

no more than 3 lines? isn't that pushing it a bit? i would say write your code in the way that makes the most sense to you and to anyone who will be maintaining your code in the future. if that means writing four lines or more, then so be it.

Answer 4

Robert Martin has some extremly good points taken to a ridiculous extreme.

Overly long functions can be hard to both read and maintain, but advocating that methods are no longer than three lines produces code that can be cryptic and hard to follow. This is what your coworker was saying.

Take 'rules' like this as a guide and try to apply them so that the original intent (clear, concise code) is not lost.

There is nothing really wrong with your one-method code, but you may want to make it clearer by returning the simplest cases first. E.g.

if (!propertyName.Contains("."))
  return dataObject; // this is the one!

// check nested stuff

Answer 5

I have learned to listen to your peers during a code review. If you cannot explain your code to them, then they certainly won't be able to maintain it next year.

This rule applies even more when your colleague is a psychopath who knows where you live. ;o)

Answer 6

too many functions means too many function names. after a certain threshold, it's very hard to tell what a function does when reading the call. you're forced to read each function's doc, adding a lot to what you have to read.

i'd say that a function has to done one thing and one only, but no less than one.

Answer 7

How about the 1-method version with a clear guard-clause at the top, and with the recursion clearly visible at the tail end of the method:

private object GetObject(string propertyName, object dataObject)
{
  int index = propertyName.IndexOf(".");
  if (dataObject == null || index == -1)
  {
    return dataObject;
  }

  Type dataObjectType = dataObject.GetType();
  PropertyInfo dataObjectProperty = dataObjectType.GetProperty(propertyName.Substring(0, index));

  ThrowExceptionIfObjectOrPropertyInexistant(dataObjectProperty, dataObjectType.FullName, propertyName);

  return GetObject(propertyName.Substring(propertyName.IndexOf(".") + 1),
                   dataObjectProperty.GetValue(dataObject, null));
}

Creating variables for a single use sometimes just creates more semantic "noise" that a person has to parse when trying to understand code. I "collapsed" a couple of one-use variables which I believe clarifies what the recursion is doing. YMMV. I also computed the index up front so you only have to scan the string a single time to see if it contains a '.' character, and not twice.

I find the version above more readable than either of your two choices, with the bonus as others point out that with the recursion entirely contained in one method, it's easier for the compiler to unroll the recursion and turn it into a loop.

Answer 8

Your peer has a point that the recursion is hidden. It's harder to follow two methods that are mutually recursive than one recursive method.

(Another issue in your methods is that you are mixing direct returns with a single exit point. If some of the code is working towards a single exit point, there should truly be a single exit point in the method.)

The method actually doesn't have to be recursive at all. As the recursion is the last thing that happens before the exit, it's just used to make a loop. You can easily make it iterative.

I used Split to get the strings before and after the first period, and I shortened some names. There is no reason to have very long descriptive names for local variables.

private object GetObject(string propertyName, object dataObject) {
   while (dataObject != null && propertyName.Contains(".")) {
      Type objectType = dataObject.GetType();
      string[] names = propertyName.Split('.', 2);
      PropertyInfo property = objectType.GetProperty(names[0]);

      ThrowExceptionIfObjectOrPropertyInexistant(property, objectType.FullName, propertyName);

      dataObject = property.GetValue(dataObject, null);
      propertyName = names[1];
   }
   return dataObject;
}