Wednesday, November 28, 2007

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.

7 comments:

Arnulfo Wing said...

Nicely said.! I like the fact that you not only make the original code clearer and easier to read, but you also introduced exception handling on the code. Thanks for sharing.

Matt Casto said...

What do you think about Microsoft's trend toward chaining calls in LINQ syntax?

Jim Holmes said...

@Matt: I've not fooled around enough with LINQ yet to have a good opinion on that. All I can say is that the same concepts should apply:

1) Chaining should not hinder readability. Beck's "Implementation Patterns" really got me re-engaging how I look at code from the readability aspect.

2) Chaining should not bypass necessary error checking. Software craftsmanship 101, IMO.

Troy DeMonbreun said...

IMHO, I think the term "chained" in this context would be better described as "nested". I understand you may be just quoting someone else's terminology.

When I think of "chaining", I think of Fluent Interfaces such as...

order
.AddFreeShipping()
.IncludeItem(15)
.SuppressTax();

...though I'm not implying that you don't think FIs can be abused, as well. :-)

Jim Holmes said...

@Troy:

Thanks SO much for the much-needed correction! I wrote that post, re-read it, edited it, and during that whole time had the feeling "Chained isn't the right word."

Nested is exactly right.

Justin Kohnen said...

Yeah, I'm guilty of doing this. And I like to tell myself "It optimized the code." But you're right, readability is more important.

Not to mention if you break it up like you did, it's a heck of a lot easier to debug.

Consider me converted.

Jim Holmes said...

@Justin: Every time I hear "It optimized the code" I want to shout "Show me the metrics!"

I think we all get a bit too fixated on optimization based upon unscientific approaches.

Write legible, correct code first. Worry about optimization second. Correctness always trumps performance!

(And I know you knew this <g>)

Subscribe (RSS)

The Leadership Journey