Inline Action Delegates to Reduce Code Duplication

I hate code duplication — spending my time copying and pasting code and then altering it slightly drives me nuts. It reminds me of a bad Michael Keaton movie.

Ew

I also hate it when I’m using methods with long signatures, but only a few need to be changed in the current method I’m working in. After a tedious afternoon of writing some of this code I ended up with something like the following:

public static int AddHeaderToSheet(ExcelWorksheet sheet, Header header, int maxCol, IDateFormatPref dateFormatter)
{
    int currentRow = 1;
    int headerLabelCol = 2;
    int headerDataCol = 3;

    ExcelRange range;

    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    range = sheet.Cells[currentRow, headerLabelCol, currentRow, headerLabelCol];
    WriteRangeWithNamedStyle(range, Resources.ReportTitle, HEADER_TITLE_STYLE);
    range = sheet.Cells[currentRow, headerDataCol, currentRow, maxCol];
    WriteRangeWithNamedStyle(range, header.ReportName, HEADER_CONTENT_STYLE);
    currentRow++;

    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    range = sheet.Cells[currentRow, headerLabelCol, currentRow, headerLabelCol];
    WriteRangeWithNamedStyle(range, Resources.StartDate, HEADER_TITLE_STYLE);
    range = sheet.Cells[currentRow, headerDataCol, currentRow, maxCol];
    WriteRangeWithNamedStyle(range, header.StartDate, HEADER_CONTENT_STYLE);
    currentRow++;

    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    range = sheet.Cells[currentRow, headerLabelCol, currentRow, headerLabelCol];
    WriteRangeWithNamedStyle(range, Resources.EndDate, HEADER_TITLE_STYLE);
    range = sheet.Cells[currentRow, headerDataCol, currentRow, maxCol];
    WriteRangeWithNamedStyle(range, header.EndDate, HEADER_CONTENT_STYLE);
    currentRow++;	

    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    range = sheet.Cells[currentRow, headerLabelCol, currentRow, headerLabelCol];
    WriteRangeWithNamedStyle(range, Resources.Location, HEADER_TITLE_STYLE);
    range = sheet.Cells[currentRow, headerDataCol, currentRow, maxCol];
    WriteRangeWithNamedStyle(range, header.Location, HEADER_CONTENT_STYLE);
    currentRow++;	

    for (int i = 1; i <= maxCol; i++)
    {
        sheet.Column(i).Width = maxCol == 2 ? 15 : 10;
    }

    sheet.View.FreezePanes(currentRow, 1);

    return currentRow;
}

Yuck.

There’s a couple things I can do to improve this. The first thought might be to create a Dictionary mapping the labels to the values and iterating through it in a foreach loop. That’s a good approach too, but not the approach that I took.

The first thing I noticed was the constant duplication of the following:

range = sheet.Cells[currentRow, startCol, currentRow, endCol];
WriteRangeWithNamedStyle(range, Resources.ReportTitle, HEADER_TITLE_STYLE);

I decided to take care of those two lines immediately with an inline Action function that looks like this:

Action<int, int, string, string> writeCells = (startCol, endCol, content, style) =>
    {
        ExcelRange range = sheet.Cells[currentRow, startCol, currentRow, endCol];
        ExcelUtil.WriteRangeWithNamedStyle(range, content, style);
    };

This allows me to use writeCells.Invoke() with starting and ending columns, the content of the cells, and the style to be used in the cells. I am also able to access currentRow and sheet without needing to pass them separately inside the function. Since the function is being declared in the method after the declaration of sheet and currentRow, both can be used freely.

The resulting code looks like this:

public static int AddHeaderToSheet(ExcelWorksheet sheet, Header header, int maxCol, IDateFormatPref dateFormatter)
{
    int currentRow = 1;
    int headerLabelCol = 2;
    int headerDataCol = 3;

    Action<int, int, string, string> writeCells = (startCol, endCol, content, style) =>
    {
        ExcelRange range = sheet.Cells[currentRow, startCol, currentRow, endCol];
        ExcelUtil.WriteRangeWithNamedStyle(range, content, style);
    };
	
    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    writeCells.Invoke(headerLabelCol, headerLabelCol, Resources.ReportTitle, HEADER_TITLE_STYLE);
    writeCells.Invoke(headerDataCol, maxCol, Header.ReportTitle, HEADER_CONTENT_STYLE);
    currentRow++;

    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    writeCells.Invoke(headerLabelCol, headerLabelCol, Resources.StartDate, HEADER_TITLE_STYLE);
    writeCells.Invoke(headerDataCol, maxCol, Header.EndDate, HEADER_CONTENT_STYLE);
    currentRow++;
	
    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    writeCells.Invoke(headerLabelCol, headerLabelCol, Resources.EndDate, HEADER_TITLE_STYLE);
    writeCells.Invoke(headerDataCol, maxCol, Header.EndDate, HEADER_CONTENT_STYLE);
    currentRow++;
	
    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    writeCells.Invoke(headerLabelCol, headerLabelCol, Resources.Location, HEADER_TITLE_STYLE);
    writeCells.Invoke(headerDataCol, maxCol, Header.Location, HEADER_CONTENT_STYLE);
    currentRow++;

    for (int i = 1; i <= maxCol; i++)
    {
        sheet.Column(i).Width = maxCol == 2 ? 15 : 10;
    }

    sheet.View.FreezePanes(currentRow, 1);

    return currentRow;
}

That got a bit smaller and more readable. I no longer need to inspect the lines to check the small little pieces that have changed. I also don’t need to ensure that I’ve changed every line in case there is a small logic change.

But, still, it’s also made the remaining repetition even more obvious. Just to be clear, I’m talking about this:

sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
writeCells.Invoke(headerLabelCol, headerLabelCol, Resources.ReportTitle, HEADER_TITLE_STYLE);
writeCells.Invoke(headerDataCol, maxCol, Header.ReportTitle, HEADER_CONTENT_STYLE);
currentRow++;

I’ll take care of that with a second inline action function that incorporates the first. This one must be in the code after the first or it will not compile. It will not have the first inline function in scope otherwise.

The action looks like this:

Action<string, string> writeHeaderRow = (key, value) =>
{
    sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
    writeCells.Invoke(headerLabelCol, headerLabelCol, key, HEADER_TITLE_STYLE);
    writeCells.Invoke(headerDataCol, maxCol, value, HEADER_CONTENT_STYLE);
    currentRow++;
};

And my final code:

public static int AddHeaderToSheet(ExcelWorksheet sheet, Header header, int maxCol, IDateFormatPref dateFormatter)
{
    int currentRow = 1;
    int headerLabelCol = 2;
    int headerDataCol = 3;

    Action<int, int, string, string> writeCells = (startCol, endCol, content, style) =>
    {
        ExcelRange range = sheet.Cells[currentRow, startCol, currentRow, endCol];
        ExcelUtil.WriteRangeWithNamedStyle(range, content, style);
    };
	
    Action<string, string> writeHeaderRow = (key, value) =>
    {
        sheet.Row(currentRow).StyleName = HEADER_CONTENT_STYLE;
        writeCells.Invoke(headerLabelCol, headerLabelCol, key, HEADER_TITLE_STYLE);
        writeCells.Invoke(headerDataCol, maxCol, value, HEADER_CONTENT_STYLE);
        currentRow++;
    };
	
    writeHeaderRow.Invoke(Resources.ReportTitle, Header.ReportTitle);
    writeHeaderRow.Invoke(Resources.StartDate, Header.StartDate);
    writeHeaderRow.Invoke(Resources.EndDate, Header.EndDate);
    writeHeaderRow.Invoke(Resources.Location, Header.Location);

    for (int i = 1; i <= maxCol; i++)
    {
        sheet.Column(i).Width = maxCol == 2 ? 15 : 10;
    }

    sheet.View.FreezePanes(currentRow, 1);

    return currentRow;
}

All of that copying and pasting has been reduced to two inline functions and four very readable lines. Any logic that needs to change will be very easy to implement without needing to make sure I’ve hit every spot.

The resultant code is not significantly smaller than the original code, but it is far more readable, and more enjoyable to write than just using Ctrl+C, Ctrl+V.

But what makes this superior to creating a separate method? Well, this way I only need to pass in the fields that are changing. There is no need to give it column positions, the worksheet, and the current row every single time. Data which is being used consistently by the actions and would clog up the parameter lists when I am calling the methods.

If there are any other thoughts on the refactoring take, I’d love to hear them.

Advertisements

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