Readability, Accuracy, and Maintainability of Chained Calls

The ability to chain together a bunch of calls is a handy thing, but it can easily lead to a number of issues in your code.

Readability's an awfully critical thing when you're writing code.  How many times have you read or heard that in almost every case you're writing code not for the compiler to read but for other developers, either yourself a few days/months/years down the road, or some other guy who has to work with your code at some point.

I'm working on a bug in some code which looks like the example below.  It's meant to return a stream of a file on the filesystem.  That file is pointed to by the Path property of a biz object we use, instantiated as contentFile in this example below.

   65 return new MemoryStream(File.ReadAllBytes(ConfigurationManager.AppSettings.Get("DataDir") + contentFile.Path));

 

A couple things hit my brain as I'm working on this.

First, the line is nearly 140 characters long with a bunch of chained calls.  That's hard to read simply because it extends well past most windows.  (At least if you're using multiple monitors and splitting windows.)  I much prefer a width of around 80 chars and won't ever go past 100.

Secondly, I much prefer having return statements returning something you've already built elsewhere.  To me that's much clearer.  Refactored and split up, the code looks like:

   65 MemoryStream contentFileStream =

   66     new MemoryStream(File.ReadAllBytes(

   67                         ConfigurationManager.AppSettings.Get("DataDir") +

   68                         contentFile.Path));

   69 return contentFileStream;

The next item is to clean up the chaining a bit.  It takes way too many brain cycles to break all these calls on lines 66 to 68 into their composite pieces.  The semantics of what you're trying to accomplish easily gets lost in all those chained calls.  A quick refactor turns this into 

   65 string filepath = ConfigurationManager.AppSettings.Get("DataDir") +

   66                   contentFile.Path;

   67 MemoryStream contentFileStream =

   68     new MemoryStream(File.ReadAllBytes(filepath));

   69 return contentFileStream;

Better, but there's still a problem in that we're using string concatenation for the construction of the filepath.  Evil since we shouldn't rely that the DataDir app setting value was properly terminated with the correct directory separator.  The lovely Path.Combine() method can help out here because it will smash together path and deal with the separators in the correct fashion regardless of whether they're there already -- and it's handled with the correct separators for whatever platform you're writing on, too.  Let's change that a bit:

   65 string filepath =

   66     Path.Combine(ConfigurationManager.AppSettings.Get("TKOPublishedFilesDir"),

   67                 contentFile.Path);

There's also a glaring error in that we're not using any guard to check whether or not the file actually exists on the system!  Let's fix that with a quick check:

   68 if (! File.Exists(filepath) )

   69 {

   70     //handle error

   71 }

Obviously you need real stuff instead of "//handle error", OK?   Also, I'll often build my test variable before the if statement but this File.Exists() is short enough that it's very readable as is.

Right now the tweaked method looks like this:

   61 public Stream GetFile(long id)

   62 {

   63     ContentFile contentFile =

   64         PolicyInjection.Create<EntityFactory<ContentFile>>().Find(id);

   65     string filepath =

   66         Path.Combine(ConfigurationManager.AppSettings.Get("DataDir"),

   67                     contentFile.Path);

   68     if (! File.Exists(filepath) )

   69     {

   70         //handle error

   71     }

   72 

   73     MemoryStream contentFileStream =

   74         new MemoryStream(File.ReadAllBytes(filepath));

   75     return contentFileStream;

   76 }

There's a bit of other cleanup which should happen here, such as wrapping the creation of the memory stream in some guards to catch exceptions, etc. I also realized that I don't like the name I gave the memory stream object, but I can will refactor that after I post this.

The main point of my post is that call chaining, statement chaining, or whatever the heck you want to call it, is a powerful thing, but it's something that easily morphs into something that leads your code into a place where it's difficult to read, hard to maintain, and susceptible to errors.

Use it with care.

UPDATE:  In the comments Troy points out that this is actually nesting, not chaining.  He's absolutely correct.  I wrote this post and the entire time was thinking "That's not the right word."  Thanks for the good feedback, Troy.