The Need to Refactor


There are times when you look at code and you know it has to be retooled. Yesterday was one of those times.

I want you to look at a bit of code. The following routine was written by a group of programmers from offshore. One theory amongst my peers is this code was purposefully written this way to get more billable hours. Another theory is the developers just don’t know any better. I am not here to worry about the reasons the code is the way it is, but rather to take you through an exercise in maintaining difficult code.

First, here is the original code:

private void WriteDiscoverFile()
{
    try
    {
        IEnumerator enumerator = null;
        StreamWriter writer = MyProject.Computer.FileSystem.OpenTextFileWriter(
this.FilePath + this.FileName, false, Encoding.ASCII);
        writer.WriteLine(this.GetFileHeaderRec().ToUpper());
        writer.WriteLine(this.Group.GetGroupHeaderRec().ToUpper());
        try
        {
            enumerator = this.Group.Batches.Values.GetEnumerator();
            while (enumerator.MoveNext())
            {
                IEnumerator enumerator2 = null;
                cBatch current = (cBatch) enumerator.Current;
                writer.WriteLine(current.GetBatchHeaderRec().ToUpper());
                try
                {
                    enumerator2 = current.Transactions.Values.GetEnumerator();
                    while (enumerator2.MoveNext())
                    {
                        cTransaction transaction = (cTransaction) enumerator2.Current;
                        writer.WriteLine(transaction.GetDetailTransactionRec().ToUpper());
                    }
                }
                finally
                {
                    if (enumerator2 is IDisposable)
                    {
                        (enumerator2 as IDisposable).Dispose();
                    }
                }
                writer.WriteLine(current.GetBatchTrailerRec().ToUpper());
            }
        }
        finally
        {
            if (enumerator is IDisposable)
            {
                (enumerator as IDisposable).Dispose();
            }
        }
        writer.WriteLine(this.Group.GetGroupTrailerRec().ToUpper());
        writer.WriteLine(this.GetFileTrailerRec().ToUpper());
        writer.Close();
        writer.Dispose();
    }
    catch (Exception exception1)
    {
        ProjectData.SetProjectError(exception1);
        Interaction.MsgBox("Error During File Write Process.  Error Description:" +
exception1.ToString(), MsgBoxStyle.OkOnly, null);
        ProjectData.ClearProjectError();
    }
}

When you first see this code, you probably wonder what is going on? How can I maintain this? But a closer look at the code gives some hints to what is happening. Let’s break it down, in English. Here are the steps:

  1. Create a writer to write out lines
  2. Write the header lines
  3. Get the enumerator from the batches collection (it is actually a SortedList object)
  4. Loop through the batches collection using the enumerator
  5. For each batch, get the enumerator for the transactions collection
  6. Loop through each transaction
  7. Write out footers
  8. Close writers

Let’s go step by step.

First, the signature leaves us where we cannot easily unit test the code. There is too much magic being pulled from the object at hand, so we change this to get rid of the magic. The result is the following signature:

private void WriteDiscoverFile(string fullFileName, bool append, Encoding encoding, Group group)
{
}

Although I am only passing the information from within the class, this allows me to mock the Group object so I can test the code in a predictable manner rather than running the production file and then examining for mistakes. 

The next few lines are nearly equivalent, although I have removed the My object, as it was a bit of overkill. In addition, the original code was magically pulling the value from the My object. This was not a huge problem, as the code was all contained in the UI project, but I am now separating out tiers, so pulling from My, which is essentially a .config wrapper in this case, broke encapsulation. That is bad, largely unmaintainable (or at least difficult to maintain) and fairly messy to work with. I won’t go further on this aside. Here are the next few lines:

SortedList<string, Batch> batches = group.Batches;
StreamWriter writer = new StreamWriter(fullFileName, append, encoding);
writer.WriteLine(GetFileHeaderRecord());
writer.WriteLine(group.GetGroupHeaderRecord());

I did pull the SortedList for batches, which was magically pulled from the object earlier. There was nothing wrong with the initial implementation, except it required spinning up the object and exposing a Set method on batches (which should be read only, as they are pulled from the database and calculated). By inverting the code a bit and allowing the method that calls WriteDiscoverFile() to send in the group, I can isolate the WriteDiscoverFile() method and test it. The writer lines, as mentioned are pretty much the same as before.

The enumerators are just a way to loop. Another fine loop (and cleaner in this case) is the for loop. Here is the bulk of the routine:

foreach (Batch batch in batches.Values)
{
   
string batchHeader = batch.GetBatchHeaderRecord();
   
SortedList<string, Transaction> transactions = batch.Transactions;
   
writer.WriteLine(batch.GetBatchHeaderRecord());

    foreach (Transaction transaction in transactions.Values)
    {
       
writer.WriteLine(transaction.ToString());
    }

    writer.WriteLine(batch.GetBatchTrailerRecord());
}

This is much cleaner and the intent is very clear, making this much easier to maintain. I then finish off writing out the trailers and get rid of the Writer, as follows:

writer.WriteLine(group.GetGroupTrailerRecord());
writer.WriteLine(GetFileTrailerRecord());
writer.Close();

Here is the entire routine:

private void WriteDiscoverFile(string fullFileName, bool append, Encoding encoding, Group group)
{
    SortedList<string, Batch> batches = group.Batches;
    StreamWriter writer = new StreamWriter(fullFileName, append, encoding);
    writer.WriteLine(GetFileHeaderRecord());
    writer.WriteLine(group.GetGroupHeaderRecord());
    foreach (Batch batch in batches.Values)
    {
        string batchHeader = batch.GetBatchHeaderRecord();
        SortedList<string, Transaction> transactions = batch.Transactions;
        writer.WriteLine(batch.GetBatchHeaderRecord());
        foreach (Transaction transaction in transactions.Values)
        {
            writer.WriteLine(transaction.ToString());
        }
        writer.WriteLine(batch.GetBatchTrailerRecord());
    }
    writer.WriteLine(group.GetGroupTrailerRecord());
    writer.WriteLine(GetFileTrailerRecord());
    writer.Close();
}

Which would you rather maintain?

Summary

What have we done here? Here is a list

  1. Removed dependencies on the My object
  2. Moved the conversion to Upper Case to the function that produces header and trailer lines (not mentioned until here)
  3. Inverted the signature a bit so we can test the code with a mock for the Group object (Testing is good)

In the end, we have 14 lines of code (getting rid of the braces) versus 30 (many of which would never be hit or were always true).

There is one question that might come to mind. Why did I remove the error handling bits? The main reason is I did not see them as necessary, as the process has run in the same location for ages. But, if I were to feel it was needed, rather than have a bunch of nested tries, I would simply surround the instantiation of the StreamWriter, as that is the only point I can see failure occurring. It would look like this:

try
{
    StreamWriter writer = new StreamWriter(fullFileName, append, encoding);
}
catch (Exception ex)
{
    //Log exception
}

Note that we can also use a using statement if we do not wish to log, and allow the failure to create file error to bubble up to the UI. It really depends on whether or not we have a true user interface or not. My vote is not, as this should be an automated process, but that is a topic for another day.

Peace and Grace,
Greg

Twitter: @gbworld

Advertisements

One Response to The Need to Refactor

  1. Viktor says:

    What else you can do with the final code:1. Make it static (move GetFileHeaderRecord() to a method parameter, or make GetFileHeaderRecord() static)If it’s on multi threaded environment:2. Lock writer to be thread safe3. Use try-catch-finally to properly close writer before method throws an error – we do not want ASP.NET to lock file for writing and to lock the whole app.Why SortedList<string, Batch> batches = group.Batches; foreach (Batch batch in batches.Values)instead of foreach (Batch batch in group.Batches.Values)?

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: