Eliminate Repetition with Action<T>
Yesterday I was looking at some old code and refactoring it to clean it up (in this case I wasn’t the original author, but I’ve written code just like this). The application in question was a simple process that had to run once per month, on demand, and so was coded up as an EXE application. As it ran, it provided updates on its progress, so it looked something like this:
static void Main(string[] args)
{
Console.WriteLine("Starting Application @ " + DateTime.Now);
var parser = new ParameterParser();
Parameters parameters = parser.Parse(args);
Console.WriteLine("Finalizing Revenue for " + parameters.DateRange.StartDate.Month + "/" + parameters.DateRange.StartDate.Year);
IRevenueRepository repository = new LinqRevenueRepository();
var mostRecentFinalizedDate = repository.GetMostRecentFinalizedDate();
if (mostRecentFinalizedDate >= parameters.DateRange.StartDate)
{
throw new Exception("Revenue already finalized for period.");
}
Console.WriteLine("Begin FinalizePublisherRevenue @ " + DateTime.Now);
repository.FinalizePublisherRevenue(parameters.DateRange);
Console.WriteLine("End FinalizePublisherRevenue @ " + DateTime.Now);
Console.WriteLine();
Console.WriteLine("Begin FinalizeAccountManagerRevenue @ " + DateTime.Now);
repository.FinalizeAccountManagerRevenue(parameters.DateRange);
Console.WriteLine("End FinalizeAccountManagerRevenue @ " + DateTime.Now);
Console.WriteLine();
Console.WriteLine("Begin FinalizeAgencyRevenue @ " + DateTime.Now);
repository.FinalizeAgencyRevenue(parameters.DateRange);
Console.WriteLine("End FinalizeAgencyRevenue @ " + DateTime.Now);
Console.WriteLine();
Console.WriteLine("Begin FinalizeAccountManagerRevenueDetail @ " + DateTime.Now);
repository.FinalizeAccountManagerRevenueDetail(parameters.DateRange);
Console.WriteLine("End FinalizeAccountManagerRevenueDetail @ " + DateTime.Now);
Console.WriteLine();
Console.WriteLine("Begin FinalizeAgencyRevenueDetail @ " + DateTime.Now);
repository.FinalizeAgencyRevenueDetail(parameters.DateRange);
Console.WriteLine("End FinalizeAgencyRevenueDetail @ " + DateTime.Now);
Console.WriteLine();
Console.WriteLine("Begin SaveAccountManagerRevenueSummary @ " + DateTime.Now);
repository.SaveAccountManagerRevenueSummary(parameters.DateRange);
Console.WriteLine("End SaveAccountManagerRevenueSummary @ " + DateTime.Now);
Console.WriteLine();
Console.WriteLine("Begin SavePublisherRevenueSummary @ " + DateTime.Now);
repository.SavePublisherRevenueSummary(parameters.DateRange);
Console.WriteLine("End SavePublisherRevenueSummary @ " + DateTime.Now);
}
In reviewing this simple application, I noticed a few areas where it could benefit from obvious improvement that wouldn’t take long at all to implement. I refactored the core logic into a separate service class, which took in an ILogger rather than writing directly to the console and also created a custom exception to take care of the case where the program was being re-run for a period that had already been finalized. But the significant improvement came in the area of logging the start/end of each method call, which I did by using an Action<T> method. You can see in the original code there was a lot of repetition – every method call requires 3 lines that are virtually identical except for the name of the method being called. Clearly a violation of Don’t Repeat Yourself. I replaced each 3-line call with a single line method call that took in an Action<DateRange> parameter representing the method to invoke. The result is that all of the magic strings for the method names disappeared, and the code is much more compact and easily understood:
public void FinalizeMonth(DateRange dateRange)
{
DateTime mostRecentFinalizedDate = _revenueRepository.GetMostRecentFinalizedDate();
if (mostRecentFinalizedDate >= dateRange.StartDate)
{
throw new RevenuePreviouslyFinalizedException("Revenue already finalized for period: " + dateRange);
}
ExecuteStep(dateRange, _revenueRepository.FinalizePublisherRevenue);
ExecuteStep(dateRange, _revenueRepository.FinalizeAccountManagerRevenue);
ExecuteStep(dateRange, _revenueRepository.FinalizeAgencyRevenue);
ExecuteStep(dateRange,
_revenueRepository.FinalizeAccountManagerRevenueDetail);
ExecuteStep(dateRange, _revenueRepository.FinalizeAgencyRevenueDetail);
ExecuteStep(dateRange,
_revenueRepository.SaveAccountManagerRevenueSummary);
ExecuteStep(dateRange, _revenueRepository.SavePublisherRevenueSummary);
}
private void ExecuteStep(DateRange dateRange, Action<DateRange> stepMethod)
{
_logger.Info("Begin " + stepMethod.Method.Name + " @ " + DateTime.Now);
stepMethod.Invoke(dateRange);
_logger.Info("End " + stepMethod.Method.Name + " @ " + DateTime.Now);
}
Note that within ExecuteStep(), I’m able to extract the name of the method being called by referring to the Action<T>.Method.Name property. This is something I wasn’t previously aware of, but it helped clean up my code significantly in this case (the FinalizeMonth method is now only about 10 lines of code, instead of the original main() method which was over 30 lines of code. The new version is also testable, and in fact I added unit tests to confirm everything works as expected, since these also were lacking from the original code. The result, after about an hour, is something much cleaner that I or another developer will be able to maintain witha greatly reduced learning curve and risk of breakage.
Comments
Kevin Kuebler said on 02 Mar 2010 at 10:14 PM
Nice refactoring. When I look at the new version, there's still one aspect of duplication that sticks out to me a little bit. The repeated calls to ExecuteStep(dateRange, Action<DateRange>). I was thinking something like this:
foreach (var step in GetSteps())
{
ExecuteStep(date, step);
}
Where GetSteps() looks something like this:
private IEnumerable<Action<DateRange>> GetSteps()
{
//Option 1
yield return _revenueRepository.Step1;
yield return _revenueRepository.Step2; //etc
//Option 2
return new List<Action<DateRange>>
{
_revenueRepository.Step1,
_revenueRepository.Step2, //etc
};
}
What do you think? Is that taking it too far? I've done this kind of thing before, but curious to see what you think.
Kevin
ssmith said on 03 Mar 2010 at 9:01 AM
@Kevin,
Sure, I can see that as an improvement. In fact, it occurs to me that going your route would also let me utilize parallel execution, with something like Parallel.For(). The first 5 steps can all be done in parallel, as could the last 2 steps. Since in reality for this particular application there's no need to optimize for performance I don't think I'd bother with the added complexity, but it's interesting to see how the design could evolve to support it (this app is also not at all cpu-bound, so all parallelizing it would do is kick off all of the database calls in parallel).