Hall of shame

We keep a "Hall of Shame" of WTF-level code snippets to remind us that however bad we might think things get, it could always be worse.  It also serves as a reminder to us that we can't be complacent with ignorance, and lack of strong technical leadership in critical applications can have far-reaching negative consequences.  Here are a few fun examples.

Runtime obsolescence

I've seen this when we have members that we don't want anyone to use anymore.  Unfortunately, it didn't quite come out right:

public static bool Authenticate(string username, string password)
{
    throw new Exception("I'm obsolete, don't call me, bro!");
}

Nobody knows not to call this method until their codebase actually executes this code at runtime, which could be at any phase in development.  If it's an obscure method, it might not get called at all until production deployment if test coverage is low.

We want to communicate to developers at compile-time that members are obsolete, and we can do this we the ObsoleteAttribute:

[Obsolete("Please use the SessionContext class instead.", true)]
public static bool Authenticate(string username, string password)
{
}

Both the IDE and compilers support this attribute to give feedback to developers when a member should no longer be used.

Exception paranoia

I've seen exception paranoia mostly when developers don't have a thorough understanding of the .NET Framework.  A try...catch block is placed around code that can absolutely never fail, but juuuuuust in case it does:

public static MyIdentity Ident
{
    get
    {
        if (((HttpContext.Current != null) && (HttpContext.Current.User != null)) && (HttpContext.Current.User.Identity != null))
        {
            try
            {
                return (HttpContext.Current.User.Identity as MyIdentity);
            }
            catch (Exception exception)
            {
                ExceptionManager.Publish(exception);
                return null;
            }
        }
        return null;
    }
}

This is also another case of TCPS.  The "if" block already takes care of any failures of the "try" block, and as we're using the "as" operator, the downcasting can never fail either.  The 13 lines of code above were reduced to only one:

public static MyIdentity Ident
{
    get
    {
        return HttpContext.Current.User.Identity as MyIdentity;
    }
}

If HttpContext.Current is null, I'd rather throw the exception, as my ASP.NET app probably won't work without ASP.NET up and running.

Clever debug switches

This is probably my favorite Hall of Shame example.  So much so that we named an award at my previous employer after the pattern.  Sometimes I like to insert a block of code for debugging purposes.  If I want my debug statements to live on in source control, I have a variety of means to do so, including compiler directives, the ConditionalAttribute, or even source control to look at history.  A not-so-clever approach we found was:

public void SubmitForm()
{
    if (false)
    {
        MessageBox.Show("Submit button was clicked.");
    }
}

To make the code run, you change the block to "if (true)".  This had us cracking up so much we named our Hall of Shame award after it, calling it the "If False Award".  Whenever we found especially heinous or corner-cutting code, they deserved the "If False Award".  We never awarded ignorance, only laziness.

Opportunities to improve

It's easy to let a "Hall of Shame" create negative and unconstructive views towards the codebase.  We always used it as a teaching tool to show that we would fix ignorance, but we would not tolerate laziness.

Every developer starts their career writing bad code.  Unfortunately, some never stop.

Stop the madness

I've been extending a legacy codebase lately to make it a bit more testable, and a few small, bad decisions have slowed my progress immensely.  One decision isn't bad in and of itself, but a small bad decision multiplied a hundred times leads to significant pain.  It's death by a thousand cuts, and it absolutely kills productivity.  Some of the small decisions I'm seeing are:

• Protected/public instance fields
• Inconsistent naming conventions
• Try-Catch-Publish-Swallow
• Downcasting

The pains of these bad decisions can wreak havoc when trying to add testability to legacy codebases.

Public/protected instance fields

One of the pillars of OOP is encapsulation, which allows clients to use an object's functionality without knowing the details behind it.  From FDG:

The principle states that data stored inside an object should be accessible only to that object.

Followed immediately by their guideline:

DO NOT provide instance fields that are public or protected.

It goes on to say that access to simple public properties are optimized by the JIT compiler, so there's no performance penalty in using better alternatives.  Here's an example of a protected field:

public class Address
{
    protected string zip;

    public string Zip
    {
        get { return zip; }
    }
}

public class FullAddress : Address
{
    private string zip4;

    public string Zip4 
    {
        get
        {
            if (Zip.Contains("-"))
            {
                zip4 = zip.Substring(zip.IndexOf("-") + 1);
                zip = zip.Substring(0, zip.IndexOf("-"));
            }
            return zip4;
        }
    }
}

There was an originally good reason to provide the derived FullAddress write access to the data in Address, but there are better ways to approach it.  Here's a better approach:

public class Address
{
    private string zip;

    public string Zip
    {
        get { return zip; }
        protected set { zip = value; }
    }
}

I've done two things here:

• Added a protected setter to the Zip property
• Changed the access level of the field to private

Functionally it's exactly the same for derived classes, but the design has greatly improved.  We should only declare private instance fields because:

• Public/protected/internal violates encapsulation
• When encapsulation is violated, refactoring becomes difficult as we're exposing the inner details
• Adding a property later with the same name breaks backwards binary compatibility (i.e. clients are forced to recompile)
• Interfaces don't allow you to declare fields, only properties and methods
• C# 2.0 added the ability to declare separate visibility for individual getters and setters

There's no reason to have public/protected instance fields, so make all instance fields private.

Inconsistent naming conventions

Names of classes, interfaces, and members can convey a great deal of information to clients if used properly.  Here's a good example of inconsistent naming conventions:

public class Order
{
    public Address address { get; set; }
}

public class Quote : Order
{
    public void Process()
    {
        if (address == null)
            throw new InvalidOperationException("Address is null");
    }
}

When I'm down in the "Process" method, what is the "address" variable?  Is it a local variable?  Is it a private field?  Nope, it's a property.  Since it's declared camelCase instead of PascalCase, it led to confusion on the developer's part about what we were dealing with.  If it's local variable, which the name suggests, I might treat the value much differently than if it were a public property.

Deviations from FDG's naming conventions cause confusion.  When I'm using an .NET API that uses Java's camelCase conventions, it's just one more hoop I have to jump through.  In places where my team had public API's we were publishing, it wasn't even up for discussion whether or not we would follow the naming conventions Microsoft used in the .NET Framework.  It just happened, as any deviation from accepted convention leads to an inconsistent and negative user experience.

It's not worth the time to argue whether interfaces should be prefixed with an "I".  That was the accepted convention, so we followed it.  Consistent user experience is far more important than petty arguments on naming conventions.  If I developed in Java, I'd happily use camelCase, as it's the accepted convention.

Another item you may notice as there are no naming guidelines for instance fields.  This reinforces the notion that they should be declared private, and the only people who should care about the names are the developers of that class and the class itself.  In that case, just pick a convention, stick to it, and keep it consistent across your codebase so it becomes one less decision for developers.

Try Catch Publish Swallow

Exception handling can really wreck a system if not done properly.  I think developers might be scared of exceptions, given the number of useless try...catch blocks I've seen around.  Anders Heljsberg notes:

It is funny how people think that the important thing about exceptions is handling them. That is not the important thing about exceptions. In a well-written application there's a ratio of ten to one, in my opinion, of try finally to try catch. Or in C#, using statements, which are like try finally.

Here's an example of a useless try-catch:

public class OrderProcessor
{
    public void Process(Order order)
    {
        try
        {
            ((Quote)order).Process();
        }
        catch (Exception ex)
        {
            ExceptionManager.Publish(ex);
        }
    }
}

In here we have Try Catch Publish Swallow.  We put the try block around an area of code that might fail, and catch exceptions in case it does.  To handle the exception, we publish it through some means, and then nothing.  That's exception swallowing.

Here's a short list of problems with TCPS:

• Exceptions shouldn't be used to make decisions
• If there is an alternative to making decisions based on exceptions, use it (such as the "as" operator in the above code)
• Exceptions are exceptional, and logging exceptions should be done at the highest layer of the application
• Not re-throwing leads to bad user experience and bad maintainability, as we're now relying on exception logs to tell us our code is wrong

Another approach to the example might be:

public class OrderProcessor
{
    public void Process(Order order)
    {
        Quote quote = order as Quote;
        
        if (quote != null)
            quote.Process();
    }
}

The only problem this code will have is if "quote.Process()" throws an exception, and in that case, we'll let the appropriate layer deal with those issues.  Since I don't any resources to clean up, there's no need for a "try..finally".

Downcasting

I already wrote about this recently, but it's worth mentioning again.  I spent a great deal of time recently removing a boatload of downcasts from a codebase, and it made it even worse that the downcasts were pointless.  Nothing in client code was using any additional members in the derived class.  It turned out to be a large pointless hoop I needed to jump through to enable testing.

Regaining sanity

The problem with shortcuts and knowingly bad design decisions is that this behavior can become habitual, and the many indiscretions can add up to disaster.  I had a high school band director who taught me "Practice doesn't make perfect - perfect practice makes perfect."

By making good decisions every day, it becomes a habit.  Good habits practiced over time eventually become etched into your muscle memory so that it doesn't require any thought.  Until you run into a legacy codebase that is, and you realize how bad your old habits were.

A downcasting tragedy

And now, for another tale of legacy code woe, here's a gut-wrenching tragedy of OCP and downcasting, in five acts.

Exposition

We have a feature we're implementing that when the store's configuration settings allow for a survey link to be shown, it should show up, otherwise, it shouldn't.  We put these settings in a simple interface (cut down for brevity):

public interface ISettings
{
    bool ShowSurvey { get; }
}

Settings can come from many places, such as configuration files, database, or web service.  This is a pretty nice example of OCP, as our app is open for extension for different "ISettings" implementations, but closed for modification as someone can't decide to use something else besides "ISettings".  Since we have an interface, the source doesn't matter.

We manage access to the settings through a static class:

public static class StoreContext
{
    public static ISettings Settings { get; set; }
}

Actual instances are set during early in the ASP.NET pipeline, so later stages have that information.  The original ISettings implementations are stored in ASP.NET Session.

Now, we need to provide functional and regression tests for this feature, using Selenium.

Rising action

To help with functional and regression tests, I'm employing the record and playback state technique I mentioned earlier.  I create an "ISettings" implementation that can be serialized and deserialized into XML, which helps when I want to manually edit the settings:

[XmlRoot("Settings")]
public class SettingsDto : ISettings
{
    public SettingsDto() { }

    public SettingsDto(ISettings settings)
    {
        ShowSurvey = settings.ShowSurvey;
    }

    public bool ShowSurvey { get; set; }
}

My "download.aspx" page combines both the cart and the settings information into a single DTO, and streams this file to the client.  In my "replay.aspx" page, I take the XML, deserialize it, and put it into Session.  Finally, I redirect to the "shoppingcart.aspx" page, which pulls the ISettings implementation out of Session and sets the StoreContext variable to my deserialized SettingsDto instance.

This is all fairly straightforward, and I'm pretty happy about this solution as I can now plug in any settings I like and verify the results through automated tests.  As it is with all legacy code, things just aren't that simple.

Climax

Running the code the first time, downloading and replaying goes smoothly, and all of my values are serialized and deserialized properly as my unit tests prove.  However, when I get redirected to the "shoppingcart.aspx" page, I get a distressing exception:

System.InvalidCastException:
  Unable to cast object of type 'BusinessObjects.SettingsDto'
  to type 'BusinessObjects.DbSettings'.

The dreaded invalid cast exception, something is making some really bad assumptions and killing my code.

Falling action

Just when I thought I was making progress, the InvalidCastException came up and crushed my dreams.  Looking at the stack trace, I see that my the exception is coming from the base Page class that all pages in our app derive from.  Here's the offending code:

public abstract class StorePage : Page
{
    protected DbSettings Settings
    {
        get { return (DbSettings)StoreContext.Settings; }
    }
}

A convenience property on the base class allowed derived classes to get at the static ISettings instance.  But on closer inspection, this property is downcasting the ISettings instance coming out of StoreContext to DbSettings.  Downcasting is when you cast to a derived type from an instance of a base type.  That's a fairly large assumption to make, though.  How do I know for sure that the instance is really the derived type, and not some other derived type of the base type?  I can't guarantee that assumption holds, as this example shows.

To make things worse, it explicitly casts instead of using the C# "as" operator (TryCast for VB.NET folks).  At least then it won't fail when trying to downcast, it would just be null.  Using the "as" operator is perfectly acceptable as a means of downcasting.  I try to avoid it, as it can be a smell of Inappropriate Intimacy.

Because the code assumes that the ISettings instance is actually seriously for realz a DbSettings instance, I can never take advantage of OCP and swap out different "ISettings" implementations.  Quite depressing.

Denouement

As an experiment, and because I have a nice automated build process, I can just try to change the type referenced in the StorePage class to ISettings instead of DbSettings.  Luckily, the build succeeded and I kept the change.

If it had failed and clients absolutely needed DbSettings, I might have to look at pushing up methods to the ISettings interface, if it's possible.  If I have access to ISettings that is.  If I don't, well, I'm hosed, and my denouement would turn into a catastrophe.

The moral of this story is:

Avoid downcasting, as it makes potentially false assumptions about the underlying type of an object.  Use the "as" operator instead, and perform null checking to determine if the downcasting succeeded or not.  We can then perform safe downcasting and avoid using exceptions as flow control.

The sinking ship

An interesting quote from a colleague today (paraphrased):

Moving developers to projects on the legacy system is like rearranging chairs on the Titanic.

We can tidy it up, but it's still going to the bottom of the Atlantic.  Probably better just to catch the plane.

It came up in the context of investing strategically versus buying tactically.  It's not an either-or question, but it's important to be thinking about long-term goals and assigning resources appropriately to match your strategic view.  The Technical Debt metaphor can offer guidance here too, as buying tactically tends to max out your credit cards (so to speak), while investing strategically actually nets returns.

Dependency Breaking Techniques: Inline Static Class

Often times I run into a class that has a dependency not on a Singleton, but a static class.  When refactoring away from a Singleton, a common approach is to use Inline Singleton.  With static classes, a slightly different approach needs to be taken because client code isn't working with an instance of a type, but rather static methods on the type itself.

Dependency breaking techniques are used to get legacy code under test.  The goal isn't necessarily to break all client dependencies out, but just the one I'm modifying to get under test.  I'm not interested changing any of the other clients that may use this static class.  Since nothing is under test, it's too risky to make non-backwards compatible changes.

The pattern

Code needs access to a method or property but doesn't need a global point of access to it.

Move Static's features to instance methods of that type.

Motivation

Helper methods tend to congregate into static classes with static methods.  Over time, the responsibility grows for the static class until it becomes a dumping ground to any behavior that seems relevant to the name.  The name of the static class usually is post-fixed by "Helper", "Manager", "Utility", or other generic and obtuse names.

Eventually, this static class will have opaque dependencies of its own, where callers don't know what's happening behind the scenes when those helper methods are called.  This can wreak havoc with unit testing, especially when trying to add tests to legacy code.

Mechanics

1. Find the static method in the calling class and use Extract Interface to extract an interface that contains only the static method being called in the client code.
2. Make the static class explicitly implement the new extracted interface.  The static class should have two methods with identical signatures, one a static method, one an explicitly implemented interface method.
3. Use Move Method to move the logic from the static method to the instance method.
4. Use Hide Delegate to delegate the static method calls to the use the instance method instead.
5. Compile and test.
6. Modify Client code using the static Operation method to instantiate an IStatic instance and call the interface Operation method.
7. Compile and test.
8. Use Extract Parameter to pass in the IStatic instance to the Client method.  If IStatic is a primal dependency, extract the parameter to a constructor argument and set the variable to a local field.
9. Compile and test.

Example

In our e-commerce system, one of the pages the user is presented with is a payments page.  It is in this page that the user decides how they want to pay for their order, whether it's credit card, invoice, financing, etc.

Not all payment types should be displayed for all carts, and there is some complex business logic that determines who sees what payment types.  There are several payment filtering strategies to encapsulate this business logic, and one type uses the user's profile to filter the payments.  Here's the strategy class:

public class AccountFilter
{
    private readonly string[] _paymentTypes;

    public AccountFilter(string[] paymentTypes)
    {
        _paymentTypes = paymentTypes;
    }

    public void AddPaymentOptions(ShoppingCart cart)
    {
        IPayment payment = ProfileManager.FindPaymentByType(_paymentTypes);

        if ((payment != null) &&
            (! string.IsNullOrEmpty(cart.AccountNumber)))
        {
            cart.PaymentFilters.Add(payment);
        }
    }

}

The business has a new requirement for us: Payment types should not be filtered using the user's profile for existing quotes.  In our system, customers can save their cart as a "quote", so that the unit cost information can be "locked" for a set amount of time.  Enabling this change requires us to change the AccountFilter class.

Being a legacy code system, the AccountFilter class has no tests defined on it.  It's our goal to make the change and add tests to verify the new requirements, and that's all.  We're not trying to add 100% unit test coverage for the AccountFilter class, just to test the changes we're making.

Not knowing if I can unit test this class, the easiest way to see if it's testable is to try using it in a test.  Here's the behavior I want, in a unit test:

[TestMethod]
public void Should_not_add_filter_when_basket_is_a_quote()
{
    ShoppingCart cart = new ShoppingCart();
    cart.IsQuote = true;
    cart.AccountNumber = "123ABCD";

    AccountFilter filter = new AccountFilter(new string[] { "CRD", "INV" });
    filter.AddPaymentOptions(cart);

    Assert.AreEqual(0, cart.PaymentFilters.Count);
}

I try running this test, and it fails spectacularly.  The error I get is along the lines of "HttpContext.Current is null", so something is dependent on the ASP.NET runtime being up.

Looking at the AddPaymentOptions method of the AccountFilter class, I notice the call to the static method FindPaymentByType on the ProfileManager class.  That method is extremely hideous, with hard dependencies on HttpContext.Current, a web service, and a database.  Instead of focusing on breaking those dependencies out, I can apply Inline Static Class and break out AddPaymentOptions' dependency on ProfileManager.  The end goal is to break the dependencies of the class I'm trying to test, not all the sub-sub-sub dependencies that may be around.

Step 1:

ProfileManager is a huge class, with about 100 different methods.  The only one I'm interested in is the FindPaymentByType method, so I'll create an IProfileManager interface that includes only that method, matching signature and name.

public interface IProfileManager
{
    IPayment FindPaymentByType(string[] paymentTypes);
}

Step 2:

Now that I have my IProfileManager, I'll make the static class implement the new interface.  Note that the static class itself isn't marked "static", only its methods.  Additionally, I'll need to explicitly implement that method so it doesn't conflict with the existing static method.

public class ProfileManager : IProfileManager
{
    public static IPayment FindPaymentByType(string[] paymentTypes)
    {
        // Hit web service for widget
        // Query database based on widget
        // Return database object
    }

    IPayment IProfileManager.FindPaymentByType(string[] paymentTypes)
    {
        return null;
    }
}

I compile, and everything looks good.  Notice I didn't add any access modifiers to the new IProfileManager method, and the inclusion of the interface name prefixed on the method name.  That's how I can explicitly implement the interface, and not get any compile errors even though these two methods have the exact same signature.

Step 3, 4:

Now I can move the existing implementation to the new interface method, and use Hide Delegate to change the static method to call the new interface method.  I don't want to introduce duplication and simply copy over the implementation, so Hide Delegate allows me to retain backwards compatibility and save a lot of work of changing all clients to use the new method.

public class ProfileManager : IProfileManager
{
    public static IPayment FindPaymentByType(string[] paymentTypes)
    {
        IProfileManager profileManager = new ProfileManager();
        return profileManager.FindPaymentByType(paymentTypes);
    }

    IPayment IProfileManager.FindPaymentByType(string[] paymentTypes)
    {
        // Hit web service for widget
        // Query database based on widget
        // Return database object
    }
}

Now my static method delegates to the instance methods, and no other clients are affected by this change.  Again, it's not my goal to fix every client dependency on this static method, only the one I'm changing.

I compile and test, but my initial unit test is failing as it's still calling the static method.

Step 6:

Now that I have an interface to depend upon instead of a static method, I'll change the client code to use the interface instead of the static method.  It looks much like the code inside the Hide Delegate of the static method:

public void AddPaymentOptions(ShoppingCart cart)
{
    IProfileManager profileManager = new ProfileManager();
    IPayment payment = profileManager.FindPaymentByType(_paymentTypes);

    if ((payment != null) &&
        (! string.IsNullOrEmpty(cart.AccountNumber)))
    {
        cart.PaymentFilters.Add(payment);
    }
}

I create a variable of type IProfileManager, instantiate it to a ProfileManager instance, and use the IProfileManager.FindPaymentByType method instead of the static method.  I compile and test, but my unit test still fails as I still haven't fully extracted the dependency on the ProfileManager, but that will be taken care of shortly.

Step 8:

I have two options, I can either pass in the IProfileManager as in the method call or I can pass it in through the constructor.  I prefer to pass dependencies through constructors over method parameters, because I like a clear separation between queries, commands, and dependencies.  Additionally, if I'm using an IoC container, I can wire up these dependencies a little more easily.  I'll use Preserve Signatures [Feathers 312] and Chain Constructors to keep the original constructor, as there are still quite a few clients of my AccountFilter class that I don't want to change:

public class AccountFilter
{
    private readonly string[] _paymentTypes;
    private readonly IProfileManager _profileManager;

    public AccountFilter(string[] paymentTypes)
        : this(paymentTypes, new ProfileManager()) { }

    public AccountFilter(string[] paymentTypes, IProfileManager profileManager)
    {
        _paymentTypes = paymentTypes;
        _profileManager = profileManager;
    }

    public void AddPaymentOptions(ShoppingCart cart)
    {
        IPayment payment = _profileManager.FindPaymentByType(_paymentTypes);

        if ((payment != null) &&
            (!string.IsNullOrEmpty(cart.AccountNumber)))
        {
            cart.PaymentFilters.Add(payment);
        }
    }
}

So I did a few things here:

• Add a new constructor that took an IProfileManager, and created the backing field for it
• Make the other constructor call the new one, passing in a new ProfileManager, which is what existing code would do
• Make the AddPaymentOptions method use the field variable instead of a local variable

What this code allows me to do is pass in a different IProfileManager instance in my test methods to verify my behavior.  I don't really care what the ProfileManager does, all I care about is what it tells me.  Somewhere down the line, I have an integration test that puts the two together, but until then, my unit test will suffice.  Here's my final unit test, which fails for the reason I want it to fail, because the assertion fails:

[TestMethod]
public void Should_not_add_filter_when_basket_is_a_quote()
{
    ShoppingCart cart = new ShoppingCart();
    cart.IsQuote = true;
    cart.AccountNumber = "123ABCD";

    string[] paymentTypes = new string[] { "CRD", "INV" };

    MockRepository repo = new MockRepository();

    IProfileManager profileManager = repo.CreateMock<IProfileManager>();

    using (repo.Record())
    {
        Expect.Call(profileManager.FindPaymentByType(paymentTypes))
            .Return(null);
    }

    using (repo.Playback())
    {
        AccountFilter filter = new AccountFilter(paymentTypes, profileManager);
        filter.AddPaymentOptions(cart);
    }

    Assert.AreEqual(0, cart.PaymentFilters.Count);
}

I use RhinoMocks to pass in a mock IProfileManager, and test-driven development to add the functionality desired.  I broke the dependency of the class under test away from the ProfileManager static method, and was able to preserve existing functionality.  By preserving existing interfaces and functionality, I can eliminate much of the risk involved in changing legacy code.  I got my changes in, was able to test them, and kept out of the rabbit-hole that a larger refactoring would have thrown me into.

The Legacy Code Dilemma and compiler warnings

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.

Legacy code testing techniques: subclass and override non-virtual members

One of the core techniques in Michael Feathers' Working Effectively With Legacy Code is the "Subclass and Override Method" technique.  Basically, in the context of a test, we can subclass a dependency and override behavior on a method or property to nullify or alter its behavior as needed.  This is especially useful if we can't extract a dependency outside of a class under test.  Mocking frameworks such as Rhino.Mocks allow me to verify interactions, or simply put in some canned responses for dependencies.

Since we're dealing with legacy code, paying down the entire technical debt at once isn't an option.  We have to make micro-payments by introducing seams into our codebase.  It might make the code slightly uglier at first, but as Feathers notes in his book, sometimes surgery leaves scars.

Some problem code

I need to override a method to provide an alternate, canned value.  However, in C#, members are not virtual by default.  Members have to be explicitly marked "virtual", otherwise subclassed members can only shadow base members.  For instance, suppose we have class Foo and Bar:

public class Foo : Bar
{
    public decimal CalculateDiscount()
    {
        return CalculateTotal() * 0.1M;
    }
}

public class Bar
{
    public decimal CalculateTotal()
    {
        // Hit database, webservices, etc.
        return PricingDatabase.GetPrice(itemId);
    }
}

I'm trying to test some class that uses Foo.CalculateDiscount method to perform some figure out what shipping methods should be available.  It uses the Foo discount to figure this out.  However, CalculateDiscount calls a base class method (CalculateTotal), which then goes and hits the database!  Not much of a unit test when it hits the database (or web service, or HttpContext, etc.).  Worse, I don't have access to the Bar class, it's in another library that we don't own.  Here's the test I'm trying to write:

[Test]
public void Should_add_shipping_option_when_discount_is_lower_than_50()
{
    MockRepository mocks = new MockRepository();

    Foo foo = mocks.CreateMock<Foo>();

    using (mocks.Record())
    {
        Expect.Call(foo.CalculateTotal()).Return(100.0M);
    }

    using (mocks.Playback())
    {
        string[] shippingMethods = ShippingService.FindShippingMethods(foo);

        Assert.AreEqual(shippingMethods[0], "OneDay");
    }
}

I could try to remove the dependency on the Foo class somehow.  But there's a lot of information that my shipping method needs, so I can't just pass in individual parameters for all of the Foo data.  The Foo class is actually quite large, so trying to extract an interface wouldn't really help either.  In any case, I just want to override the behavior of the "CalculateTotal" call, and since it's not marked "virtual", I can't override the behavior directly.  For example, this won't work in my test:

public class FooStub : Foo
{
    public new decimal CalculateTotal()
    {
        return 100.0m;
    }
}

The shipping service knows about "Foo", not "FooStub".  Since the FooStub.CalculateTotal is shadowing the Bar method, only when I have a variable of type FooStub will its CalculateTotal get called.  Since the shipping service knows Foo, it will use the Bar CalculateTotal method, even if I pass in an instance of a FooStub.  Shadowing can cause weird things like this to happen, to I try to avoid it as much as possible.

Subclass and override

So subclassing and overriding, one of the main functions of mocking frameworks, won't work at the test level.  I need to get the override on or between the Foo and Bar classes.  Why don't we create a new BarSeam class between the Foo and Bar classes?

public class Foo : BarSeam
{
    public decimal CalculateDiscount()
    {
        return CalculateTotal() * 0.1M;
    }
}

public class BarSeam : Bar
{
    public virtual new decimal CalculateTotal()
    {
        return base.CalculateTotal();
    }
}

public class Bar
{
    public decimal CalculateTotal()
    {
        // Hit database, webservices, etc.
        return PricingDatabase.GetPrice(itemId);
    }
}

The BarSeam class now sits between Foo and Bar in the inheritance hierarchy.  BarSeam shadows the Bar.CalculateTotal, with one key addition, the "virtual" keyword.  By making BarSeam.CalculateTotal virtual, I can now subclass Foo and override CalculateTotal, put in a canned response, and not worry about hitting the database.

Yes, BarSeam is ugly, but I want to bring ugly to the front of the class to make sure it doesn't get swept under the rug.  Since all client code either references Foo or Bar, no client code will be affected as BarSeam's default implementation merely calls the base method.  Think of it as Adapter, but instead of incompatible interfaces, I'm dealing with a non-extensible interface.

Conclusion

When working with legacy code, it's not feasible (or even wise) to try and rewrite the application.  By making these micro-refactorings and introducing seams, we're able to identify places in need of larger refactorings, as well as meeting our original goal of getting our legacy code under test.