Separating Concerns in Methods

I'm a big fan of readable code. And I especially like it when there are clear pathways of functionality in code. Sometimes, things don't come out quite right the first time, so we need to work on it. This happened to me a couple weeks ago (First Tries are Sometimes Ugly). Fortunately, code is not carved in stone; we can go back and keep improving it.

The code that we're looking at is designed to get the sunset time from a service. Once we have this value, we create home automation events relative to that time -- for example, having lights come on 30 minutes before sunset or 1 hour after sunset.

All of this code (and the articles) are collected here: Rewriting a Legacy Application. The completed code for this article is in commit 9e3ad90f33 of the GitHub project: jeremybytes/house-control.

A Bit of a Mess
It's good to get help from folks who have experience with stuff that you don't. I was able to tap into Matt Johnson to help me fix the date/time parsing issues that I had (details: Code Gets Better by Sharing Experience and Expertise). But that was just one part that needed fixing.

Here's how we last left the "GetSunset" method which is part of the "SunriseSunsetOrg" project:

This has the date/time parsing updates in it, so it isn't as ugly as it was, but it still needs some help.

Here are the items that need work (as noted in the prior article):

• We probably want to split out functionality into separate methods (service call, JSON parsing, date/time manipulation).
• The Lat/Long values need to be a bit more dynamic (probably coming from configuration).
• We should probably be "await"ing our asynchronous methods rather than locking the thread.
• We need better error handling.
• I'm pretty sure there's a better way to get to the "sunset" value in the JSON data.
• And the parsing of the time string... I'm not sure what it needs, but it definitely needs something.

We can cross off the last item. But let's fix some of these other things -- primarily the organization of the code.

Separation of Concerns
The biggest problem I have with the method is that it is doing too many things. So I split things out a bit. In doing this, I also created a bit of a fluent API, and we'll see this as we go along.

So, here's the new "GetSunset" method:

Yep, that's it. I've split out the functionality into separate methods. This gives us smaller methods that are responsible for doing only one thing. This makes things easier to debug, maintain, and test in the long run.

New Feature: A Local Cache
You'll notice that the first thing we do is call a "RefreshCache" method. Since we are making a service call to get our sunset data, it makes sense to only do this once per day. If we need to make multiple calls, then we can just use the data that we collected earlier.

The cache is pretty simple: just 2 fields. The "cacheData" field holds the string value that comes back from the service (the JSON result in this case). The "cacheDate" is the date that the data refers to.

So, to refresh the cache, we just check to see if our "cacheDate" is the same as the date that we're looking for. If it is, then we can just use the "cacheData" that we already have. Otherwise, we make the service call to update the data.

As a side note, the commented out line is a JSON result from the service. This is so we can easily uncomment this line and comment the next line if we don't want to hit the service while we're in development mode.

Concern #1: Calling the Service
The first concern that we split into its own method is calling the service. This is the "GetContentFromService" method:

This method holds the functionality to get the JSON result from the service. For this, it creates an HttpClient, configures the query string, and then gets the results.

There are a few things we want to look at here: first, our "using" statement that wraps the "HttpClient". "HttpClient" implements the IDisposable interface, so it's a best practice to wrap this with a "using" statement. This makes sure that the Dispose method is called appropriately when we're done with the object.

A nice thing about moving this to a separate method is that the scope for the "using" is much smaller. If we look at the original method, we see that the "using" block encompasses the entire method. But since we've split this functionality out, the "using" block can be scoped more appropriately.

Another thing to notice are the "await"s inside the method. The "GetAsync" and "ReadAsStringAsync" methods are asynchronous (as you might guess from the names). Previously, we were checking the "Result" of these methods. This has the effect of locking up the thread while we wait for the calls to finish. When we "await" the methods, the thread is free to continue processing requests while we're waiting for the asynchronous methods to return. This is especially useful when we're making network calls since it usually takes some time to establish the connection and transmit the data across the wire.

And notice that since we're "await"ing methods, we have marked the "GetContentFromService" methods as "async" and it returns a "Task<string>" (rather than just a string). For more information on consuming asynchronous methods, check out Task and Await: Consuming Awaitable Methods.

Waiting for a Result
We're not taking full advantage of the asynchronous method calls. Lets go back to the "RefreshCache" method:

This just gets the "Result" from the asynchronous method. So we do end up turning this back into synchronous code. And that's what we need in this case.

Concern #2: Parsing JSON
The second big concern in our original method was to parse the JSON result that comes back from the service. We need to be able to pick out the "sunset" value from this object. And that's what we do in the "GetSunsetString" method:

This is pretty much the same code that we had before; it's just separated into its own method. This is responsible for deserializing the JSON object and then getting the value of the "sunset" field.

As a reminder, this is a string like "1:06:09 AM". This is a UTC time value that needs to be converted into a local date/time. And that's our next concern.

Concern #3: Parsing UTC Time
To parse the string time value, we have another method -- GetLocalTime:

This has the code that Matt helped us out with before. It takes the UTC string value and turns it into a DateTime object with the local sunset time. (Check the previous article for a detailed description.)

A Fluent API
I made a decision to create a fluent API with some extension methods here. Both "GetSunsetString" and "GetLocalTime" are extension methods on the string object. (For more information on extension methods, take a look at "Quick Byte: Extension Methods".)

This may not be the best direction to go. The methods themselves are very targeted. For example, "GetSunsetString" doesn't really extend any "string" object, just string objects that contain the JSON object that we're expecting.

In the same way "GetLocalTime" will only work with strings that contain a UTC time value. These aren't re-usable outside of this class.

But they do allow us to write code like this:

And I kind of like this. "cacheData" contains our JSON string, so we can call "GetSunsetString" on this object. Since the result of "GetSunsetString" is a UTC time string, we can just pass the result on to the "GetLocalTime" method.

I'm not completely sold on this. But I'll stick with it for the time being.

[Update 1/26/2015: The more I look at these extension methods, the less I like them. I'll explain why in an upcoming article... And here it is: Fixing Problems with a (Specific) Fluent API.]

Re-Using These Methods
One of the great things about having the concerns separated out into their own methods is that we can compose them in different ways. For example, I added a method that knows how to get sunrise times as well:

We have to do pretty much the same steps if we want to get a "sunrise" time instead of a "sunset" time. So we call "RefreshCache" (which calls "GetContentFromService" if needed). And we use the same "GetLocalTime" method.

The "GetSunriseString" method is a little different because it needs to get a different value out of the JSON result, but the rest of the functionality (including the cache) is shared.

Relative Schedules
To make use of the ability to get sunset and sunrise times, I modified the schedule objects a bit to support relative times. This included some other refactoring as well. Here are the objects that we have now:

Inside the "ScheduleInfo" object (I really hate that name, by the way -- that needs to be changed), there is a "ScheduleTimeType" property. This is an enum with the values "Standard", "Sunset", and "Sunrise". In addition, I added a "RelativeOffset" so that we can schedule items relative to sunset or sunrise.

Here's are some schedule items (from our "Schedule.txt" file):

The 2nd and 3rd items are scheduled relative to "Sunset". The 2nd item is 30 minutes before sunset, and the 3rd item is 1 hour after sunset.

When we run our application and check the schedule items, we can see the relative values:

At the top, we can see that the sunset time is "5:16:46 PM" (local). Notice that the 2nd item is scheduled for "4:46:46 PM" -- 30 minutes prior to sunset, and the 3rd item is scheduled for "6:16:46 PM" -- 1 hour after sunset.

So we can see that the relative schedules are working and that they are getting the sunset value from our service.

In addition, if we set a breakpoint inside the "GetContentFromService" method, we'll see that the method is only called 1 time (even though this calls "GetSunset" 3 times). This lets us know that the cache is working as expected.

How Are We Doing?
So we have working code. That's good. I put this code into production, and the items are rescheduling as expected. But let's go back and review the items that we want to fix:

We probably want to split out functionality into separate methods (service call, JSON parsing, date/time manipulation).
CLOSED: We took care of this one. We have separated out the concerns into separate methods. This will make things easier to debug and enhance. And as we saw, we can re-use the methods by composing them in different ways.

The Lat/Long values need to be a bit more dynamic (probably coming from configuration).
OPEN: I'm not overly concerned about this particular item since this software only runs at a single location, but this would be a "nice to have".

We should probably be "await"ing our asynchronous methods rather than locking the thread.
CLOSED: We added the "await"s to our HttpClient calls. We still turn this back into synchronous code, but we need to do that at some point for this code to work as expected.

We need better error handling.
PENDING: Error handling is almost non-existent, but we do check for status and null along the way. If we have a problem -- with the service call or the parsing -- then the method simply returns the value for midnight. We'll need to take a look at the application as a whole to see how we want to handle an error condition here.

I'm pretty sure there's a better way to get to the "sunset" value in the JSON data.
OPEN: I still need to do some more research on this one (or find a JSON expert to give me some advice).

And the parsing of the time string... I'm not sure what it needs, but it definitely needs something.
CLOSED: We've saw how much better the code got previously with some help from Matt.

Wrap Up
So overall, we're in much better shape than when we started. I'm still working on getting the tests in place for this functionality. And I'm also looking at different approaches to the same problem. What would happen if I had approached this entire functionality using TDD? There may be some interesting issues since we're making a service call and a big part of figuring things out was how to work with the service. I'll probably experiment with this as part of a coding practice session in the future.

Readable code is vital to keeping our technical debt manageable. Often we end up throwing code together just to get something that works. But we need to make sure that we don't leave it that way. If I had left the ugly code and come back to it 6 months later, it would have taken me a while to figure out what it is doing. But now that the different pieces of functionality are split out into their own methods, its much easier to see what's going on and how the various pieces fit together.

We need to remember that the code we write is just as much for the human developers as it is for the compiler and computer.

Happy Coding!