The Legacy Code Dilemma and compiler warnings

This post was originally published here.

I hit the Legacy Code Dilemma today while trying to reduce compiler warnings in our solution.  For those that don’t know it, the Legacy Code Dilemma is:

I need to fix some legacy code, but the fix isn’t ideal.

After working on green-field development for a while, dealing with legacy code can be frustrating at first, unless you go through Michael Feathers’ excellent Legacy Code book.  Whenever I feel the need to spend two days refactoring a small area to get it under test, I have to remind myself that it’s more important to deliver business value AND pay down technical debt than paying down technical debt alone.  But then you hit dilemmas, where you KNOW the solution you’re putting in is ugly, but just don’t have the resources to “do it right”.

To fix or not to fix

I hit that today, where I was trying to reduce compiler warnings.  I could fix the compiler warnings by making them go away, or fix the design problems that made the compiler warnings show up in the first place.

For example, I encountered a lot of “variable declared but not used”:

try
{
    return PricingDatabase.GetPrice(itemId);
}
catch (Exception ex)
{
    return 0.0M;
}

In this case, the specific warning is “The variable ‘ex’ is declared but never used”.  The problem is the variable “ex” is declared but never referenced in the “catch” block.  The design flaw, at least one of them, is that exception handling is used for flow control.  Any time you see try-catch block swallow the exception and do something else, it’s a serious code smell.

The problem about fixing the design issue is that I’m dealing with legacy code, which has no tests.  Such design changes are foolish and even dangerous without proper automated tests in place.

Live to fight another day

If I wanted to fix the design flaw, I was in for a real battle.  I would need to get this module under test, and get any client modules under test.  For a codebase with little or no unit tests in place, that’s just not feasible.

Instead, I’ll fix the warning:

try
{
    return PricingDatabase.GetPrice(itemId);
}
catch (Exception)
{
    return 0.0M;
}

I kept the exception swallowing and all its nastiness, but gained a small victory by removing a compiler warning.  Since this solution has several hundred compiler warnings, all these little victories will pay off.  Instead of charging off on a path of self-righteous refactoring, I’ve fixed the pain and lived to tell the tale.