DaedTech

Stories about Software

By

How to Lose Friends and Alienate Programmers

The Road to Madness is Paved with Lying APIs

Today, I’d like to discuss the subject of little things that add up to making your code a chore to work with for others. I’m not talking about things like code that’s incomplete or non-functional, or other obvious problems, but rather subtle, sort of insidious ones. The obvious problems result in you going to the person in question and demanding a fix or else simply rolling your own version instead. The insidious ones result in hidden wasted time, undue frustration, and the need to head down to the local pub to take the edge off after a long, annoying day. Unlike broken/missing/whatever code, these are not immediately obvious, and they can drive you nuts by making it unclear where the problem really lies.

A few posts ago, I mentioned a piece of code that contained an API with a quirky method call:

public void DoSomething(bool shouldPerform)
{
   if(shouldPerform)
   {
       //A whole bunch of processing
   }
}

I changed the name of the parameter there for the sake of illustration in that post. Below, I’ll show a more close approximation of the actual API at play, and leave out the implementation details. The code in question is designed to implement undo and redo functionality. As a client of the service, you log an object and the name of a property that you want to record, and the service encapsulates storing those values, along with the ability to do conceptual undo and redo operations.

Here is more or less what the API involved (modified to some degree for the sake of distilling the issues I’ll be discussing and to understand what the parameters are actually supposed to do – their actual names don’t make this clear, which goes to the point of this post in and of itself):


public bool StartTransaction(string transactionName);
public bool StartTransaction(string transactionName, object record, string recordProperty)
public void EndTransaction(bool isTransactionStarted);
public void Record(object record, string recordProperty);
public void Undo();
public void Redo();

(There are a bunch more overloads, but those aren’t really relevant to what I’m going to post here about this particular API).

When I first interact with a piece of code as a client, my inclination is to trust the author of the code. So, if I’m using a service interface that says GetFooById(int id), I don’t go inspecting the internals of the implementations of that interface — I assume that it’s going to get me a Foo, given the Foo Id. I only start inspecting the internals if there is unexpected behavior or a problem in general.

I don’t think that’s an unreasonable policy. Perhaps it’s a bit generous at times, but I think that if more people made this assumption, more people would code to that standard, and more would get done.

With this code, what I wanted to do was write a facade for it, since this was implemented not as an actual interface, but as a hybrid of static/singleton/monostate, where some of the methods were static and some were instance methods, but all operated on the same static/global state. I didn’t want my service and presentation tier classes interacting with this directly, so I wrapped the functionality in an implementation and had it implement an interface for the Start/End transaction and Record() bit.

So, I set about coding my facade and letting that inform the particulars of the interface, and then I coded the rest of the classes against my interface so that I could proceed in TDD fashion. That way, the only untested thing when integration time came was the actual facade implementation. So, when it came time to integrate, I fired up the application and ran it, and… everything went wrong.

Thinking back on this a few days later with some perspective, I realized that my interaction with and troubleshooting of my facade created a “teachable moment” of sorts, for myself, and hopefully in general. The problem lay in my misunderstanding of the implementation (which does work in production, if not in a way that is obvious at all through the API), but the problem also lay, subtly, in what I had to do in order to gain the proper understanding. The first part is on me, the second part is on the service author. As to whether I should have taken the API at face value or not, I’d say that’s a matter of individual opinion.

Parameters that Don’t Matter

After getting all of my tests to pass, firing up the application to test the integration, and experiencing disaster, the first thing I did was spend a good bit of time going over my assumptions for my new code. After all, my code was new, and the service code was pre-existing and functional. After wasting a good bit of time eliminating this as a possible issue, I turned next to my facade and its interaction with the service API. Everything there seemed reasonable as well, so I resigned myself to having to examine the implementation details of the service in order to understand what it was actually doing.

The first thing that I discovered was that there were lots of overloads, both public and private, and the result of this was a deep stack between the API and the actual execution code. It took me a while, but I wrote down the various parameters and traced them all the way through to their eventual execution locations. At this point, I had to double and triple check my work because I discovered that, when calling StartTransaction(string transactionName), it made absolutely no difference what I passed in. This was my first moment of sympathy for Charlie Brown, as shown above.

That’s not to say that the parameter transactionName wasn’t used. It sure was. It was passed all over the place, and it was set in a field in one of the classes of the service. But, that field was never operated on. Not once. The method was demanding a parameter of me that was used, but irrelevant. This is substantially worse than unused, which CodeRush would have shown me immediately. Instead, I had to dig through the bowels of an existing service to discover that the parameter was used in the sense of being read and stored, but not used in the sense of being useful.

This is one of the little things I mentioned that sows the seeds of mistrust and creates frustration. I based my implementation on the notion that the service was associating individual transactions with their names, since this parameter was non-optional. I thus created a class level constant for my registering my particular transactions, which turned out to be a useless activity. Not a big deal, but some time wasted, which creates frustration.

Unnecessary Behaviors Forced On Clients

Another thing that I noticed in my closer examination of the service implementation’s internals was the method I blogged about earlier, EndTransaction(bool). When I first noticed the API, I chuckled a bit, but after working with it and then digging into it, I came to better understand what was actually going on here. It wasn’t simply that I had to pass the method a “true” if I wanted it to execute. The client class expected me to keep track of whether it had started a transaction or not, and to pass that state flag back to it.

This is similar in concept to an API that demonstrates a temporal dependency by forcing you to use the first method’s return value as a parameter to the second message, but with a subtle and important difference. In a method like that, the return value of the first method tends to be a by-product of the operation going on in the first method. Say, for instance, that I have an API that has a method “int SpawnProcess()” and another method “void KillProcess(int)”. This informs me that SpawnProcess spawns a process and returns a means of identifying that process, and the end method forces me to pass the int back so that it knows which process to kill. Getting it right isn’t a fifty/fifty shot as it is with a boolean — the API tells me that the EndProcess(int) expects a specific value that I should somehow know about.

The subtlety lies in the nuance of what the API is saying. The API I just described says “hey, I start a lot of processes, so I’ll start one and tell you about it, but you need to keep track of the id of your process”. The API with the boolean says “hey, I’ll tell you whether or not starting this transaction succeeded, but I’m too lazy to keep track of it myself, so I need you to tell me later.” That is, the first one has a good reason (practical infeasability without creating a context depeendency) for not keeping track of that id, whereas the second one doesn’t. EndTransaction(bool) implies that there are only two possible states. Why is it my responsibility as a client to keep track of the service’s encapsulated state for it? You can’t expose a property, or since passing in false is a no-op, just encapsulate that state flag and no-op?

The reasons I dislike this are layered. First of all, it’s counter-intuitive. Secondly, it leaks the implementation details of the service. As a client, I not only know that I start and end transactions, which I should know, but also that transaction start and end are global, static flags. Thirdly, it foists its implementation on me and tries to force me to partner with it in leaking implementation details. That is, it doesn’t just leak the details — it forces me as a client to do its internal dirty work of state management.

Getting the Metaphor Slightly Wrong

The two previous items made me pause and mildly annoyed me, but were relatively easy to figure out and correct within my facade. I had (understandably, I think) misinterpreted the API and was relatively quickly able to correct this (by passing in string.empty for the transaction name because it didn’t matter, and by keeping track of the state of the service in the facade to prevent exceptions). This third problem was the one that caused me a few additional wasted hours of debugging and squinting at service code.

If I look at the API, I’m sure I’m not alone in thinking of a metaphor of a database. The “transaction” nomenclature practically screams it. And, in the database world, I can perform operations one at a time or I can start a transaction, perform a batch of them, and then commit or rollback my batch atomically. In spite of the fact that I saw “End” instead of “Commit” or “Rollback” (and, in retrospect, this should have been a red flag), I assumed that behavior here was the same.

So, in my facade, I implemented a scheme where normal operations were recorded sans transaction and I started/ended transactions in situations where one value changing automatically changed another. To me, this was an undo “transaction”, since I wanted those to be rolled back with a single “Undo/Ctrl-Z”. Well, it turns out I was partially right in my assumption. Transactions were, in fact, atomic in nature. But, after a lot of debugging and squinting, I found that transactions were not actually optional. Every Record() operation required a transaction to be started and stopped. The metaphor was just enough like a database to create the expectation that the abstraction would mirror its behavior, but not enough like a database API where that actually worked. It would sometimes throw exceptions about a transaction and sometimes simply do nothing.

Okay, fair enough, I bracketed every Record operation with a transaction, and things were actually kind of working in the application at this point. Operations would undo and redo, except that I had to hit undo at least twice to undo every one thing I did. Now for this, I couldn’t rely on the exception debugging or anything like that to point me in the right direction, so I was really immersed in the debugger and code to figure this one out (and it’s a doozy). If you look at the two public StartTransaction() overloads, one appears to start a transaction and the other appears to start a transaction and record the first entry of the transaction. This struck me as very odd until I realized that all Records() had to occur inside of a transaction, and then I figured it was probably tacked on shorthand to prevent endless sequences of Start(); Record(); End(); for a single record.

What was really going on under the hood was that both Start() overloads invoked a common third, private overload, and passed in all of their parameters plus some. The first invocation passed in nulls for the recording target object and the property value, while the second overload passed in what you gave it. In both cases, the values were recorded in a transaction, but in the case of the Start(string) one, it recorded a pair of nulls in the stack. That’s right. The second overload wasn’t a convenience — it was the only way not to be wrong. The first method call was always wrong, by definition. So, StartTransaction(string) should really have been called StartTransactionAndAddSpuriousRecord(string) and the second overload should have been called StartTransactionAndRecordActualRecord(string, object, string). Or, better yet, the first one shouldn’t have existed at all.

In the book Code Complete, Steve McConnell discusses the importance of forming consistent abstractions:

Provide services in pairs with their opposites. Most operations have corresponding, equal and opposite operations. If you have an operation that turns a light on, you’ll probably need one to turn it off. If you have an operation to add items to a list, you’ll probably need one to delete an item from the list…. … When you design a class, check each public routine to determine whether you need its complement.

null

In the case here, we don’t have Start() and End(), but StartAndDoOtherStuff(), DoMoreOfThatOtherStuff() and End(). These are not complementary abstractions — they’re confusing, overlapping abstractions. Imagine working in C# with a list with this API:

  • List(T firstItem)
  • Add(T item)
  • Clear()

If you want an empty list, what do you think you do? Create a new list? Pshaw. Create a list, and then clear it, of course, since creating a list with no elements is impossible for some reason. Now, imagine if Microsoft pulled a switcheroo on you and foisted this new list API on you after years of working with the list you are familiar with it. My bet is that you’d feel a little like poor Charlie Brown.

Am I Overreacting?

Yes and no. The fact that I’m blogging about it probably implies that I’m more frustrated than I actually am. I don’t blame anyone for writing this code, per se, and I’m sure I’ve written code at times that’s made people pull their hair out as well — we’re all guilty of it at some time or another. An interface that seems intuitive to the author may make no sense to someone else.

Now, don’t get me wrong. I was frustrated at the time. But, I filed that frustration away, and took the opportunity instead to analyze exactly what went wrong. What we had there was a failure to communicate…

I tried to consider objectively my role in it versus the role of the service. I think that my role in it was to make assumptions without verifying them, based on the advertised API, as I mentioned in the lead-in. But, I think that it’s incumbent on green field developers and maintenance programmers alike to pull back and consider the abstractions. Are you forcing clients to pass in parameters that you don’t need? Do your functions do what they say they’re going to do? Do your functions force unnecessary behavior on clients? Do your functions misrepresent a common abstraction? If the answer to these questions is, or even may be, “yes”, it’s probably time to rework the public API for clarity.

Whoever is to blame in the matter, the fact is that I spent some hours debugging something, where I would have spent none doing so had the interface worked the way that I assumed it worked by inspecting it (once I had ferreted out my incorrect assumptions, everything worked flawlessly and my TDD tests and other unit tests proved to be correct, given my own facade API). That is frustrating and wasteful. I feel somewhat vindicated by the fact that I would have wasted zero time if the API had worked in a manner consistent with itself, whereas with the state it was in, I would have had to read/debug the internals whether I assumed anything or not, given that the API was not consistent with the actual behavior of the implementation.

So, the point of all this is that an API that does not faithfully represent the implementation is going to cause frustration. If you’re a developer working on an API that somebody else will use, do both of you a favor and look for these types of issues and any others you can think of. Don’t code up a class where some other class you’re writing at the same time is the only client. That’s a recipe for clunky interfaces and seams. And, please, write some tests, TDD tests, unit tests, whatever. These force you to use your API in another context. If the code in question here had been unit tested, I find it unlikely that it would have had its public API left in this state. Bearing these things in mind will keep your stock high with fellow programmers and have them trusting code that you write, instead of knowing that every time they try to kick an extra point, you’re going to rip the ball away and cause them to fall flat on their backs.

By

Chain of Responsibility

Quick Information/Overview

Pattern Type Behavioral
Applicable Language/Framework Agnostic OOP
Pattern Source Gang of Four
Difficulty Easy

Up Front Definitions

N/A

The Problem

Let’s say that you’re doing something reasonably standard. You have some desktop application that deals with a persistence structure (file, web service, remote database, whatever) and displays things for the user. Since it’s not 1996 anymore, you don’t want to treat the user to an endless series of dialog boxes for feedback, so you decide to create some kind of status message scheme, whereby there is a piece of screen real estate that is updated with messages as processing occurs, to give the user feedback. Think of an install wizard:

You get some requirements that you’re to have three types of status messages: informational, warning, and error, to be displayed in black, yellow and red text, respectively. From your extensive experience as a developer, you know that business analysts and users love nothing more than to tinker with the GUI, so you decide that you need a better scheme than simply displaying text and changing its color — you need to encapsulate the message generation logic so that when it is decided that the error messages should really be more of a brick red, you don’t have to do a giant find and replace in your presentation layer. So, you create the following object to which the GUI will bind:

    public class Message
    {
        public virtual string Text { get; set; }
        public Color Color { get; set; }
        public FontWeight Font { get; set; }
    }

Further, you create some classes that understand how to populate these bindable objects:

    public abstract class MessageGenerator
    {
        public const int LowPriority = 0;
        public const int Normal = 1;
        public static int Warning = 2;
        public static int Error = 3;
        public static int Critical = 4;

        public abstract Message GenerateMessage(string messageText);
    }

    public class InformationalMessageGenerator : MessageGenerator
    {
        public override Message GenerateMessage(string messageText)
        {
            return new Message() { Color = Colors.Black, Font = FontWeights.Normal, Text = messageText };
        }
    }

    public class WarningMessageGenerator : MessageGenerator
    {
        public override Message GenerateMessage(string messageText)
        {
            return new Message() { Color = Colors.Yellow, Font = FontWeights.SemiBold, Text = messageText };
        }
    }

    public class ErrorMessageGenerator : MessageGenerator
    {
        public override Message GenerateMessage(string messageText)
        {
            return new Message() { Color = Colors.Red, Font = FontWeights.Bold, Text = messageText };
        }
    }

Now, when the error message needs to be a little lighter than brick red, but a little darker than normal red, and maybe with a hint of purple, you can go to one place, the ErrorMessageGenerator, and you’re done. This is good, except that generating these things will be a little tedious. You’re going to have to instantiate the specific generator that you want and use it to generate your message. So, all over the code, you’re going to have things like this:

    public class CustomerViewModel : INotifyPropertyChanged
    {
        private CustomerService _customerService;

        private Message _statusMessage;
        public Message StatusMessage { get { return _statusMessage; } set { _statusMessage = value; RaisePropertyChanged("StatusMessage"); } }

        public void DoSomething()
        {
            var myCustomer = _customerService.GetCustomerById(12);
            if (myCustomer == null) //Oops, let's tell the GUI about this
            {
                var myGenerator = new ErrorMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Customer not found!");
            }
            else
            {
                var myGenerator = new InformationalMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Located customer 12.");
            }
        }
....

Generating those objects is sort of annoying, and it also defeats the purpose of having an abstract base class. You could always factor the generation into some view model base, but you don’t get around the problem of having to tell the new method what kind of error generation you want, but that only works for view models that inherit from a base view model. Another option would be to create a factory method or instance that generated the appropriate generator… but, generating generators seems like overkill here. It would also be sort of redundant and wrong feeling to do that, as the information about how to generate messages is all stored inside of the MessageGenerator hierarchy, but you’d be introducing some external class to figure out how these things are wired. So, you decide to leave well enough alone and plow ahead with this not quite elegant, but good enough implementation.

Now, let’s say some time goes by, and your message coloring scheme is such a hit that the users/BAs want every color in the rainbow and even some ultra-violet and infra-red messages. I mean, there are messages that are critical, and then messages that are CRITICAL. The latter should be in red, but with hints of white, to evoke images of a hospital. And, there are really several different kinds of warnings – those that you can ignore, and those that are almost errors, which should be an angry orange. There should also be messages that are green to say “Good job, user!”

You’re glad that your code is such a hit, but dismayed to see your “DoSomething()” method and all those like it ballooning in size like there’s no tomorrow:

        public void DoSomething()
        {
            var myCustomer = _customerService.GetCustomerById(12);
            if (myCustomer == null) //Oops, let's tell the GUI about this
            {
                var myGenerator = new ErrorMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Customer not found!");
            }
            else if (myCustomer.IsValid == false)
            {
                var myGenerator = new WarningMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Customer not in valid state!");
            }
            else if (PositiveFeedback)
            {
                var myGenerator = new FeelGoodMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Way to go!");
            }
            //...
            else
            {
                var myGenerator = new InformationalMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Located customer 12.");
            }
        }

Now, since this is GUI code, some of this ballooning is unavoidable – if there are 20 different things that can be displayed for customer retrieval, you’re going to need at least 20 lines for 20 strings. You might rethink the factory method at this point, for generating the generators or even the messages, thus saving yourself some lines of code, but it won’t be a huge gain in terms of the complexity of these methods. So, you forget the factory method and resign yourself to this complexity.

But, let’s say that a new wrinkle comes along. Some of your users think that the BAs got a little carried away with all of the colors, and they just want the old red/yellow/black scheme back. Others love the colors. So, you’re tasked with coming up with a way to please both sets of users. There is a “rich colors” setting in a config file that you’ll read in and, if that’s enabled, you’ll display all of the additional colors, but if not, then the first three.

Now, things get away from the realm of “unavoidable complexity for displaying lots of information to the user” and into “downright hideous” territory:

        public void DoSomething()
        {
            var myCustomer = _customerService.GetCustomerById(12);
            if (myCustomer == null) //Oops, let's tell the GUI about this
            {
                var myGenerator = new ErrorMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Customer not found!");
            }
            else if (myCustomer.IsValid == false)
            {
                var myGenerator = new WarningMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Customer not in valid state!");
            }
            else if (myCustomer.IsSemiValid)
            {
                if (RichColors)
                {
                    var myGenerator = new WishyWashyMessageGenerator();
                    StatusMessage = myGenerator.GenerateMessage("Well, you're almost there!");
                }
                else
                {
                    var myGenerator = new InformationalMessageGenerator();
                    StatusMessage = myGenerator.GenerateMessage("Well, you're almost there!");
                }
            }
            else if (PositiveFeedback)
            {
                if (RichColors)
                {
                    var myGenerator = new FeelGoodMessageGenerator();
                    StatusMessage = myGenerator.GenerateMessage("Way to go!");
                }
                else
                {
                    var myGenerator = new InformationalMessageGenerator();
                    StatusMessage = myGenerator.GenerateMessage("Way to go!");
                }
            }
            //...
            else
            {
                var myGenerator = new InformationalMessageGenerator();
                StatusMessage = myGenerator.GenerateMessage("Located customer 12.");
            }
        }

Now, you’re duplicating code in a big way. Using a factory method here could stop the bleeding somewhat, but not completely. You’ll see why when you learn what the BAs have in store for you next. When you were able to shoehorn in the selective color scheme from the last release, they got the taste of your blood in their mouths and they liked it.

You see, customers in Miami like teal messages because it’s one of the Dolphins’ colors, while males over 50 seem to be partial to muted shades of gray, and we also need a universal beige when the application is in safe mode. So, the application now needs to be able to read in all of those settings and selectively enable disable those message generators. Now, even your factory method solution is convoluted and nasty. It needs to be in a class with a bunch of booleans or take those booleans as parameters. It may buy you a little reprieve in lines of code, but it won’t buy you any help with your ominously growing complexity. Abandon all hope, ye who enter the presentation tier.

… well, okay, before you start dusting off the resume or pitching version 2.0 to management, there is another way, if you’re up for a bit of refactoring.

So, What to Do?

This is where the Chain of Responsibility pattern enters the picture. If you pull back for a moment and think of what really needs to happen here, it isn’t simply a matter of picking a message generator implementation to use. It’s a matter of prioritizing the message creators. What you’ve really got is a set of rules like “If X and Y, use creator A, otherwise, if Z, use creator B, etc”. You’ve been successful at not duplicating the actual construction of the message by using the generator polymorphs, but you are duplicating the prioritization logic.

So, first things first. Let’s introduce a concept of message priority to the creation method, since it’s already sort of defined in the base class anyway:

    public abstract class MessageGenerator
    {
        public const int LowPriority = 0;
        public const int Normal = 1;
        public static int Warning = 2;
        public static int Error = 3;
        public static int Critical = 4;

        public abstract Message GenerateMessage(string messageText, int severity);
    }

Now, each implementer has additional information that it can use to decide whether or not to bother processing a message:


    public class InformationalMessageGenerator : MessageGenerator
    {
        public override Message GenerateMessage(string messageText, int severity)
        {
            if (severity == Normal)
            {
                return new Message() { Color = Colors.Black, Font = FontWeights.Normal, Text = messageText };
            }
            return null;
        }
    }

Now, the scheme is a bit different. Each generator may or may not return a message. This doesn’t exactly help you, but it’s interesting. You’ve introduced the notion that a generator can examine a passed in parameter and then choose or decline to generate a message. To fully implement Chain of Responsibility, and solve the problem, it is now necessary to teach your generators how to pass the buck.

The idea is to give your generators a sense of “I’ll handle this message if it’s appropriate, and if it’s not, the next guy will handle it.” This, combined with some heuristics for which generators handle which priorities will move a lot of those unsightly if conditions out of your code. So, let’s start with the base class:

    public abstract class MessageGenerator
    {
        public const int LowPriority = 0;
        public const int Normal = 1;
        public static int Warning = 2;
        public static int Error = 3;
        public static int Critical = 4;

        public MessageGenerator Next { get; set; }

        public virtual Message GenerateMessage(string messageText, int severity)
        {
            return Next != null ? Next.GenerateMessage(messageText, severity) : null;
        }
    }

Here, you’ve defined a concept of next generator that can be set by clients. The formerly abstract method is now virtual to encapsulate the buck passing so that clients don’t have to do it. By default, you’ll get buck passing, and you now have to opt in to handling the message yourself. The clients will now look as follows:


    public class InformationalMessageGenerator : MessageGenerator
    {
        public override Message GenerateMessage(string messageText, int severity)
        {
            if (severity == Normal)
            {
                return new Message() { Color = Colors.Black, Font = FontWeights.Normal, Text = messageText };
            }
            return base.GenerateMessage(messageText, severity);
        }
    }

Now your code is looking promising for saving you a lot of bloat. The new message generator examines a message, handles the generation if the severity parameter is appropriate and passes if not. The real slick part of this is that the burden of message generation is still removed from the client, but so too can the burden of prioritizing message generation handling. Here’s what the new client code looks like:


    public class CustomerViewModel : INotifyPropertyChanged
    {
        private CustomerService _customerService;

        private MessageGenerator _messageGenerator;

        public CustomerViewModel(CustomerService customerService, MessageGenerator messageGenerator)
        {
            _customerService = customerService;
            _messageGenerator = messageGenerator;
        }

        private Message _statusMessage = null;
        public Message StatusMessage { get { return _statusMessage; } set { _statusMessage = value; RaisePropertyChanged("StatusMessage"); } }

        public void DoSomething()
        {
            var myCustomer = _customerService.GetCustomerById(12);
            if (myCustomer == null) //Oops, let's tell the GUI about this
            {
                StatusMessage = _messageGenerator.GenerateMessage("Customer not found!", MessageGenerator.Error);
            }
            else if (myCustomer.IsValid == false)
            {
                StatusMessage = _messageGenerator.GenerateMessage("Customer is not in valid state!", MessageGenerator.Warning);
            }
            else if (myCustomer.IsSemiValid)
            {
                StatusMessage = _messageGenerator.GenerateMessage("Well, you're almost there!", MessageGenerator.WishyWashy);
            }
            else if (PositiveFeedback)
            {
                StatusMessage = _messageGenerator.GenerateMessage("Way to go!", MessageGenerator.FeelGood);
            }
            //...
            else
            {
                StatusMessage = _messageGenerator.GenerateMessage("Located customer 12.", MessageGenerator.Normal);
            }
        }
    ...

Wow, that looks much better. One condition, one message — the cyclomatic complexity is now order linear with the actual complexity of the conditions that need to be reported, instead of condition complexity and business analyst imagination complexity multiplied. And, as a nice bonus, you’re not instantiating generators anymore – you’ve got only one that you use, and it’s injected as a dependency. Furthermore, this is accomplished without any factory method. Going back to the original example (error/warning/info), here’s the wireup for it:


    public class Wireup
    {
        public MessageGenerator BuildMessageGeneratorChain()
        {
            return new ErrorMessageGenerator() 
            { 
                Next = new WarningMessageGenerator()
                { 
                    Next = new InformationalMessageGenerator() 
                    {
                        Next = new DefaultMessageGenerator() 
                    } 
                }
            };
        }
    }

(Sorry about the nested conditional initializers — it’s late, and for some reason that amused me a lot more than it should have. It’s like some kind of code ugly-chic.) So, in this simple wireup, you have the chain of responsibility setup with some DefualtMessageGenerator defined to handle all messages not handled elsewhere. I won’t bother picturing it, but this would simply return a message every time, should the responsibility be passed to it. This would ensure that the message was always at least generated, if not with colors and fonts and whatnot.

Now, you’ve really separated responsibilities. The ViewModel and other clients don’t need to worry about message generation – it’s now a black box to them. They have a generator and the generator gives them messages from their passed in string/severity pairs. They have a “Bob’s your uncle” attitude toward the generator’s chain of delegation. And now, when the business analysts get crazy with the colors, you have exactly one place to make changes in existing code — the wireup class (you may need to add new generators as well). This is powerful because one of the more desirable situations for a code base is one in which requirements changes mean adding new code instead of modifying existing code. You can’t introduce regressions to brand new code, after all.

The only exception to not modifying existing code may be a need to periodically revisit the priorities. For instance, if every color in the rainbow has been added to the code base, you may have your default black/yellow/red handle priorities 0-10, 11-20, and 21-30 respectively, but giving the custom colors a bite at the apple first. That allows a teal message of priority 4 to be handled with TealGenerator for Miami customers, but the basic generator for all others simply by omitting the teal generator for the responsibility chain for non-Miami customers. But, if you have this scenario and the 31st color is introduced, you may need to expand the scope of the defaults and shuffle the priorities a bit.

Of course, other schemes are possible too. The generate method could take on another parameter, or you could use a different numbering scheme. The important part is that you’re creating multiple objects that can handle a request using different criteria, with easily customizable ordering scheme.

A More Official Explanation

The original Gang of Four definition of the pattern was:

Avoid coupling the sender of a request to its receiver by giving more than one object a chance to handle the request. Chain the receiving objects and pass the request along the chain until an object handles it.

The algorithm in question calls for a recursive handling of requests. A scheme is setup whereby a client invokes some service to satisfy a request, and the service does so by walking a pre-determined graph of potential handlers until one agrees to service the request. The aforementioned decoupling occurs because the client is oblivious to which potential servicer will actually handle it.

Polymorphism is a convenient mechanism for this paradigm but certainly not the only one. The only requirement is a customizable chain of handlers be given the chance to do so, and given that chance in response to a single request from the client.

Other Quick Examples

Here are some other scenarios that seem as though they’d lend itself well to Chain of Responsibility:

  1. Event handling in the a GUI context (WPF works this way, for instance, with routed events). A control can raise an event that will bubble up the logical tree until some other control handles it.
  2. Modeling a situation with approvers or handlers. For instance, consider a software scheme for managing vacation/PTO requests at a business. Approvers might include managers, VPs, etc. Requests may be handled by person X unless person X is out of office, and then they are handled by person Y, etc.
  3. In a sense, exception handling works this way — code generates an exception, and each layer of the call stack (either explicitly or implicitly) opts to handle the exception or let it bubble up.

A Good Fit – When to Use

This pattern is good to use any time the scheme for handling a request is somewhat involved and should be configurable on the fly. In the example in this post, the pattern wasn’t really important until the scheme for message handling started to get pretty customized and involved. Generally, when this is happening, Chain of Responsibility should enter the discussion.

Likewise, if there is a situation where multiple parties should have input to a request/event, the pattern may be a good idea. With a bit of modification, the example above could be used to generate composite messages (for instance, one message generator could take the message returned by another and make it bold).

Square Peg, Round Hole – When Not to Use

As always, there is the YAGNI princple. Jeff Atwood says

Don’t use fancy OOP features just because you can. Use fancy OOP features because they have specific, demonstrable benefit to the problem you’re trying to solve. You laugh, but like Rico, I see this all the time. Most programmers never met an object they didn’t like. I think it should be the other way around: these techniques are guilty until proven innocent in the court of KISS.

You could substitute “Design Patterns” for “fancy OOP features” here and the message would remain valid. In the example here, if all messages were black and with normal font, it wouldn’t make any sense to use Chain of Responsibility just in case the BAs wanted other colors. Wait until they add one or two before jumping to that conclusion and implementing this pattern. (I have a rule of thumb, which is to wait for at least two and usually three similar requests before deeming the request a pattern and adjusting accordingly. But, as soon as you hit three, do a 180 and bank on more such requests).

In addition, this pattern may not make sense if a rigid ordering is acceptable. For example, if you’re modeling a department structure, and all requests are always approved by a manager and then the VP, it may not be necessary to line up the handlers just yet. If that is unlikely ever to change, there’s no need for a complex design pattern where a simple && will suffice.

Finally, don’t confuse this with assembly. If you’re modeling the construction of a house, you don’t want a “Chain of Responsibility” where one handler builds the basement, and passes it on, the next adds a ground floor and passes it on, etc. That’s an inappropriate metaphor and it will be confusing in the long run. The house construction has a rigid ordering and isn’t in response to anything. This is a creational situation, and Chain of Responsibility is a behavioral pattern. I realize that we were, in the example, ‘constructing’ a message, but the purpose of that construction is to figure out who would build the message — not to partition the message’s construction into different spheres of responsibility (there’s another pattern for that).

So What? Why is this Better?

Chain of Responsibility is an improvement where appropriate because it introduces flexibility. It separates the handling of an event or servicing of request from the place where the event/request is generated. The requester doesn’t need to take care of this itself — it can delegate that. In the example above, the View Model class is responsible for displaying information to the user. But, before implementing the pattern, the View Model was developing a greater and greater responsibility for figuring out how to construct a message. By delegating that responsibility elsewhere, the class was simplified. By using Chain of Responsibility to do it, the whole application was made more flexible by virtue of the fact that the scheme for message construction was made easily swappable on the fly.

This pattern is not a hard sell in terms of utility, given that it’s already widely in use within frameworks, such as in the aforementioned cases of GUI events in WPF and exceptions in general.

By

You’re Doin’ It Wrong

I was chatting with someone the other day about some code that he had written, and he expressed that he wished he had done it another way. He said that there was a bunch of duplication and that, while he wasn’t sure what he should have done, he didn’t think it seemed quite right. I took a look at the code and was inclined to agree, though I could think of how I might have approached it.

This led me to think a little more broadly about writing code that you feel not quite right about. One might simply call this a “code smell”, but I think a distinction can be made. I personally associate code smells more with code from which you are somewhat removed, either because someone else wrote it or because you wrote it some time back. That is, you wouldn’t write code that “smelled” to you if it smelled as you were writing it (hopefully).

So, what I’m documenting here is not so much ways to look at code and “smell” that something is wrong, but rather a series of heuristics to keep in mind during development to know that you’re probably doing something wrong. Code smells are more macroscopic in nature with things like “parallel inheritance hierarchy” and “anemic domain model”. These are practices that, as I’ve learned from experience, seem to lead to bad code.

Typing the same thing again and again

The incident that made me think of this involved a piece of code that looked something like this:

public class SomeViewModel
{
    int CustomerId1 { get; set; }
    string CustomerName1 { get; set; }
    int CustomerId2 { get; set; }
    string CustomerName2 { get; set; }
    int CustomerId3 { get; set; }
    string CustomerName3 {get; set; }
    ....
    
    public void GetCustomers()
    {
        CustomerId1 = _SomeService.GetCustomer(1).Id;
        CustomerName1 = _SomeService.GetCustomer(1).Name;
        ...
     }

     ....
}

Now, there are a few things going on here that could certainly be improved upon, but the first one that jumps out to me is that if you’re appending numbers to fields or properties in class, you’re probably doing something wrong. We could certainly take our IDs and names and turn them into arrays, or lists or collections and iterate over them. But, even doing that, you’d have a bunch of parallel loops:

public class SomeViewModel
{
    List CustomerIds { get; set; }
    List CustomerNames { get; set; }
    
    public void GetCustomers()
    {
        CustomerIds = _SomeService.GetCustomers().Select(c => c.Id);
        CustomerNames = _SomeService.GetCustomers()Select(c => c.Name);
     }
}

Each place that you want to do something with customer IDs, you’ll also have to do it with names. This gets particularly ugly if you decide you want to sort the lists or something like that — now you have to make sure the lists are rearranged in order – you take on responsibility for guaranteeing that your multiple list implementation remains consistent. This is repetitive still. Better to create a customer model struct with properties name and ID or something, and expose a list (or observable collection or whatever) of that.

The heuristic I can offer here is that if you’re typing the same thing over and over again, you’re probably doing it wrong. This is an offshoot of the DRY (Don’t Repeat Yourself) principle, but ironically, I think it bears some repeating here. DRY doesn’t simply mean that you shouldn’t copy and paste code, which is what many people remember. It doesn’t even mean that you shouldn’t duplicate code. Dave Thomas calls it something “much grander”:

DRY says that every piece of system knowledge should have one authoritative, unambiguous representation. Every piece of knowledge in the development of something should have a single representation. A system’s knowledge is far broader than just its code. It refers to database schemas, test plans, the build system, even documentation.

There should be no duplication of knowledge in the project, period. As such, typing the same thing over and over again has the whiff of duplication, even if you aren’t copy/pasting. If you perform a series of operations on customer ID 1 and then customer name 1, and then customer ID 2, and then customer Name 2, etc, you’re repeating that operation over and over again, albeit with a nominally different target. But, what if in the database or lower layers of memory, I switch customers 1 and 2? The 1 operation is then done on 2 and vice versa. These operations are interchangeable – we have duplicated knowledge. And, duplicated knowledge eventually leads to things getting out of sync as the system evolves, which means maintenance headaches, code rot, and defects.

Making a private member public and/or an instance method static

Let’s say that we have the following classes:

public class Customer : INotifyPropertyChanged
{
   private int _id;
   public int Id { get { return _id; } set { _id = value; NotifyChange(() => Id); }

   private string _name;
   public string Name { get { return _name; } set { _name = value; NotifyChange(() => Name); } }

  //PropertyChange Elided
}

public class CustomerProcessor
{
    private int _processedCount;

    public void ProcessCustomers(IEnumerble customers)
    {
        if(customers != null)
        {
            customers.ToList().ForEach(cust => this.DoSomethingToCustomer(c));  
            _processedCount += customers.Count();
        }
    }

    public bool IsFinished()
    {
        return _processedCount > 12;
    }
}

Now, let’s say that the GUI is bound directly to the customer object, and that a new requirement comes in – we want to display customer stuff in black, unless most of the customers have been processed (say, 3/4s of them), in which case we want to display them in red — for all customers. Let’s also assume that we have some kind of value converter that converts a boolean to red/black for binding. Here’s how someone might accomplish this:

public class Customer : INotifyPropertyChanged
{
   private int _id;
   public int Id { get { return _id; } set { _id = value; NotifyChange(() => Id); }

   private string _name;
   public string Name { get { return _name; } set { _name = value; NotifyChange(() => Name); } 

   public bool IsRed { get { return CustomerProcessor._ProcessedCount > 9; } }
}

  //PropertyChange Elided
}

public class CustomerProcessor
{
    public static int _processedCount; //This is now a public global variable

    public void ProcessCustomers(IEnumerble customers)
    {
        if(customers != null)
        {
            customers.ToList().ForEach(cust => this.DoSomethingToCustomer(c));  
            _processedCount += customers.Count();
        }
    }

    public bool IsFinished()
    {
        return _processedCount > 12;
    }
}

Don’t do that! That might be the quickest and dirtiest way to accomplish the task, but it sure is ugly. We break encapsulation by making an internal detail of the processor public, we create a global variable to get around the issue that the customer has no reference to the processor, and we’ve only created a half-way solution because the customer still has no mechanism for knowing when the processor’s count passes the magic number 9 – we’d need to expose a global event or callback so that the customer would know when to raise property changed on its color. This is also a concrete and circular coupling – CustomerProcessor and Customer now know all about one another and might as well be one static, global mashup.

Private members are private for a reason — they’re implementation details hidden from the code base at large. By having the magic number 12 and the processed count hidden in the processor, I can later change the scheme by which the processor evaluates “finished” without breaking anything else. But, the way I’ve mangled the code here, the requirement that “red” be 3/4s of the finished count is now leaked all over the place. If I go to the processor and decide that finished count is now 400 (or better yet, configurable and read from persistence), our new requirement is no longer satisfied, and as the maintenance programmer for CustomerProcessor, I have no way of knowing that (except maybe for the hint that someone exposed my private variable).

Likewise, instance members are instance members for a reason. If I later decide that I want two separate customer processors working in two separate screens, I’m now hosed with these changes. However many processors I create, if any of them process 12 records, they’re all finished.

Far too often, the reason for making privates public or instance members static is convenience and not wanting to think through dependencies. Both of those make the information that you want easier to get at. But, I submit that if this is your solution to get your objects to talk to one another, you’re likely creating a short-sighed and misguided solution. In our example above, exposing an “IsAlmostFinished()” instance member on the processor and then having the GUI bind to something that knows about the customers and their processor is a better solution. It requires a bit more planning, but you’ll be much happier in the long run.

You can’t describe a method in a simple sentence using its parameters and return value

“Too many parameters” is considered a code smell. But, how many is too many? Recently, I stumbled across a method that looked something like this:

public void DoSomething(bool shouldPerform)
{
   if(shouldPerform)
   {
       //A whole bunch of processing
   }
}

This method doesn’t have too many parameters at all, I wouldn’t think. It has exactly one parameter, which is basically the programmatic equivalent of the ubiquitous “Are you sure you want to…” message box. But, I couldn’t for the life of me summarize it in a simple sentence, and perhaps not a paragraph. It was something like “If the user passes in true, then find the hardware processor and hardware presenter, and if each of those isn’t null, find the database manager, but if the hardware presenter is null, then create a file, and…”

If you’re writing a method, do a thought exercise. If someone stopped by and said, “whatcha doin?”, could you look up at them and quickly say “I’m writing a method that…”? Or would you say, “go away and don’t break my concentration, or I’ll never remember what this thing was supposed to do”? If it’s the latter, you’re doing it wrong.

We can only process so much information at once. If you’re reading through someone else’s code or your own code from a while ago, methods that are small and focused are easy to understand and process mentally. Methods that have dozens of parameters or do dozens of things are not. It doesn’t matter how smart you are or how good at programming you are – when you encounter something like that, you’ll put your pen to the monitor and start tracing things or take that same pen and start taking notes or drawing diagrams. If you have to do that to understand what a method does, that method sucks.

Consider the following interfaces:


public interface ICustomerDao 
{
    void DeleteCustomer(Customer customer);
    void InsertCustomer(Customer customer);
    void UpdateCustomer(Customer customer);
    Customer GetCustomerById(int customerId);
    IEnumerble GetAllCustomers();
}

public interface ICustomerService
{
    bool LogCustomerDetails(Customer customer);
    Customer MergeCustomers(Customer customer1, Customer customer2);
    Customer FindCustomer(IEnumerable customerForSearch);
}

There are no method definitions at all – just signatures. But, I’d be willing to be that you can already tell me exactly what will happen when I call those methods, and I bet you can tell me in a simple, compact sentence. Wouldn’t it be nice if, when you were coding, you only encountered methods like that? Well, if you think so, do others and your future self a favor, and only define and code them like that.

Defining class behavior for one specific client

Remember that method that I showed above? The one with the “are you sure” boolean parameter? I’m pretty sure that was neither the author’s idea of a joke nor was it quite as silly as it looked. I’d wager dollars to donuts that this is what happened:


public class Unmaintainable
{
    public bool IsSomethingTrue() { ... }

    public void WayToBig()
    {
        //....
        var myExtracted = new ExtractedFromTooBig();
        myExtracted.DoSomething(IsSomethingTrue());
    }
}

public class ExtractedFromTooBig
{
    public void DoSomething(bool shouldPerform)
    {
       if(shouldPerform)
       {
           //A whole bunch of processing
       }
    }
}

When looked at in this light, it makes sense. There was some concept that a class was too large and that concerns should be separated. Taken as a gestalt, it makes sense, but simply looking at the extracted class, “DoSomething(bool)” looks pretty dumb.

While the effort to refactor to smaller units is laudable, creating an interface that only makes sense for one client is not so good. Your classes should make sense on their own, as units. If you extract a class that seems hopelessly coupled to another one, it’s probably time to reconsider the interface, or the “seam” between these two classes. In the case above, the boolean parameter need not exist. “IsSomethingTrue()” can exist in an if condition prior to invocation of the parameterless “DoSomething()” method. In general, if your class’s public interface doesn’t seem to make sense without another class, you’re probably doing it wrong. (This is frequently seen in “helper” classes, which I mentioned in this post about name smells).

By

Coding Conventions and Ralph Waldo Emerson

Convention or Improvement

I was reading (and commenting on) a blog post by Jimmy Bogard at Los Techies yesterday. The basic premise was that, since Systems Hungarian Notation has basically been rejected by the developer community at large, it is interesting that we continue to pre-pend an “I” to interfaces in C#.

The post and some of the comments touched on the idea that while, yes, this is incongruous with eschewing systems Hungarian, we should continue to do it because it’s what other programmers expect and not following the convention makes your code less intuitive and readable to other C# developers. Jimmy says:

The train for picking different naming styles for interfaces and generic type parameter names left the station a long, long time ago. That ship has sailed. Picking a different naming convention that goes against years and years of prior art…

In the interest of full disclosure, I follow the naming convention and understand the point about readability, but it led me to consider a broader, more common theme that I’ve encountered in my programming career and life in general. At what point do we draw the line between doing something by convention because there is a benefit and mindlessly doing something by convention.

Emerson

Ralph Waldo Emerson famously said, “A foolish consistency is the hobgoblin of little minds.” This is often misquoted as “Consistency is the hobgoblin of little minds,” which, in my opinion, frames my question here well. The accurate quote that includes “foolish” seems to suggest that following convention makes sense, so long as one continually justifies doing so, whereas the latter misses the subtlety and seems to advocate iconoclasm for its own sake, which, one might argue, is a foolish consistency in and of itself. We all know people that mindlessly hate everything that’s popular.

Lest I venture too far into philosophical considerations and away from the technical theme of this blog, I’ll offer this up in terms of the original subject for debate. Is the fact that a lot of programmers do something a valid reason to do it yourself, or is that justification an all purpose cudgel for beating people into following the status quo for its own sake? Is the convention argument a valid one of its own right?

Specifics

This is not the first incarnation in which I’ve seen this argument about convention. I’ve been asked to name method variables “foo” instead of “myFoo” because that’s a convention in a code base, and other people don’t prepend their variables with “my”. Ostensibly, it makes the code more readable if everyone follows the same naming scheme. This is one example in the case of countless examples I can think of where I’ve been asked to conform to a naming scheme, but it’s an interesting one. I have a convention of my own where class fields are pre-pended with underscores, method parameters are camel case, and method variables are pre-pended with “my”.

This results in code that looks as follows:

internal class Customer
{
    private string _ssn;
    internal string Ssn { get { return _ssn; } }
    
    internal bool Equals(Customer customer)
    {
        var myOtherSocial = customer.Ssn;
        return _ssn == myOtherSocial;
    }
}

There is a method to my madness, vis-a-vis readability. When I look at code that I write, I can tell instantly whether a given variable is a class field, method parameter, or method variable. To me, this is clear and readable. What I was asked to do was rename “myOtherSocial” to “otherSocial” thus, in my frame of reference, losing the at-a-glance information as to what is a method parameter and what is a local variable. This promotes overall readability for the many while reducing readability for the few (i.e. me).

That’s an interesting tradeoff. One could easily make the Vulcan argument that better readability needs of the many outweigh the needs of the few. But, one could also make the argument that the many should adapt to my convention, since it provides more information. Taking at face value that I’m right about my way being better (only for argument’s sake – I’m not arguing that there is a ‘right’ in a matter of personal readability and aesthetics) is the idea of convention — that a lot of people do it the other way — a valid argument against improvement?

Knowing When Conventions Can be Ignored

I don’t presume to know what the best naming scheme for local variable or interfaces is. In fact, I’d argue that it’s likely a matter of aesthetics and expectations. People who have a harder time mentally switching contexts are going to be more likely to dig in, and mile-a-minute, flighty thinkers will be more likely to get annoyed at such ‘pettiness’ and ‘foolish consistency’. Who’s right? Who knows – who cares.

Where the rubber meets the road is how consistency or lack thereof affects a development team as a whole. And, therein lies the subtlety of what to do, in my mind. If I am tasked with developing some library with a small, public facing API for my part in a team, it makes the most sense for me to write code in a style that makes me most productive. The fact that my code might be less readable to others will only be relevant at code reviews and in the API itself. The former is a one and done concern and the latter can be addressed by establishing “public” conventions to which I would then adhere. Worrying about some hypothetical maintenance programmer when deciding what to name and how to case my field variables is fruitless to a degree because that hypothetical person’s readability preferences are strictly unknowable.

On the other hand, if there is a much more open, collaborative paradigm where people are constantly modifying one another’s code, then establishing conventions probably makes a lot of sense, and following the conventions for their own sake would as well. The conventions might even suck, but they’ll be consistent, and probably save time. That’s because the larger issue with the consistency/convention argument is facilitating efficiency in communication — not finding The One Try Way To Do Things.

So, the “arbitrary convention” argument picks up a lot of steam as collaboration becomes more frequent and granular. I think in these situations, the “foolish consistency” becomes an asset to practical developers rather than a “hobgoblin of little minds.”

Be an Adapter Instead of an Evangelizer

All that said, the conventions and arguments still annoy me on some level. I can recognize the holistic wisdom in the approach while still being aggravated by being asked to do something because a bunch of other people are also doing it. It evokes images in my head of being forced to read “Us Weekly” because a lot of other people care how many times Matt Damon went to the gym this week. Or, more on subject, it evokes images of being asked to prepend “str” to all my string variables because other people think that (redundant in Visual Studio) information makes things more readable.

But, sometimes these things are unavoidable. One can go with it, or one can figure out a way to make all parties happy. I blogged previously about my solution to my own brushes with others’ conventions. My solution was a DXCore plugin that translated my conventions to a group’s, and vice-versa. I’m happy, the group is happy, and I learned stuff in the process.

So, I think the most important thing is to be an adapter. Read lots of code, and less will throw you off your game and cause you to clamor for conventions. Figure out ways to satisfy all parties, even if it means work-arounds or interesting solutions. Those give your brain some exercise anyway. Be open to both the convention argument and the argument for a new way of doing things. Go with the flow. The evangelizers will always be there, red faced, arguing that camel case is stupid and only pascal case makes sense. That’s inevitable. It’s not inevitable that you be one them.

(Image compliments of: http://geopolicraticus.wordpress.com/2009/01/28/short-term-thinking/ )

By

More and More With TDD

TDD Revisited Again

Previously I blogged about TDD with some reservations and then later as I re-introduced myself to the pure practice as I had learned it some many moons ago. For the last month, I have been plowing ahead methodically with this experiment, and I thought I’d revisit some of my thoughts from earlier.

Reinforced Pros

First off, the pros still stand in my mind, but I’d like to add one enormous additional pro, which is that you get code right the first time. I mean, really, really right. Right as in coding for a few days without ever running the application you’re working on, and then firing it up and having it work flawlessly the first time you run it.

And, I’m not talking about plodding along with some tiny development exercise like scoring a bowling game. I mean, I recently wrote an application that would parse a text file, format and filter records, and insert the result in a database. Using a combination of Moq for my internal seams and Moles for external concerns (file and database), I exhaustively tested everything I wanted to do as I went, following the red-green-refactor process to the letter. After some hours here and there of working on this (shaking off my TDD rust being part of the time spent), I finally hit F5 and it worked. My files were parsed, my databases updated, my log file written accurately. This wasn’t a fluke, either, because I repeated the process when I wrote another utility that piggy-backed on this first one to send out emails with the results. I wrote an API for setting up custom mail messages, dependency injection for different mail clients, and wireup code for integrating. Never once did I step through the debugger trying to figure out what network credential I had missed or which message was missing which formatting. I put the finishing touches on it with my tests passing and code coverage at 100%, hit F5 and had a well-formed, perfect message in my inbox before I even looked away from my screen, expecting to be kicked into the debugger. That is a remarkable pro.

I mean, think of that. If you’re a skeptic about TDD or unit testing in general, you’ve probably never experienced anything like that. If you have, that’s amazing. I don’t know anyone that doesn’t develop in this fashion that has this happen. I mean, people who are really sharp get it mostly right, and sometimes just have to tweak some things here and maybe step through a few functions a few times, but without ever running the code? I’m not buying it. And even if they did, I come out way ahead this way. A non-tested code base is going to be harder to modify. I have hundreds of passing tests now that will tell me if subsequent changes break anything.

Cons Mitigated

So, I’d like to take a look back at the cons, and revisit my opinions after a month of doggedly sticking with TDD:

  1. Larger design concepts do not seem to be emergent the way they might be in a more shoot from the hip approach — figuring out when you need a broader pattern seems to require stepping out of the TDD frame of mind
  2. There is no specific element of the process that naturally pushes me away from the happy path — you wind up creating tests for exceptional/edge cases the way you would without TDD (I guess that’s not strictly a con, just a lack of pro).
  3. It becomes easier to keep plodding away at an ultimately unworkable design – it’s already hard to throw work you’ve put in away, and now you’re generating twice as much code.
  4. Correcting tests that have been altered by changing requirements is magnified since your first code often turns out not to be correct — I find myself refactoring tests as often as code in the early going.
  5. Has the potential to slow development when you’re new to the process — I suppose one might look at this as a time investment to be paid off when you get more fluid and used to doing it.

Regarding (1) and (3), this seems not to be the issue that I thought it would be. I had grown used to a fast flurry trying out a class concept, and realizing what refactorings needed to be done on the fly, causing it to morph very quickly. I had gotten very proficient at ongoing refactoring without the red-green preceding it, so, in this way, I was able to ‘fake’ the TDD benefit. However, I realized a paradoxical benefit – TDD forced me to slow down at the beginning. But rather than really slowing me down on the whole, it just made me toss out fewer designs. I still refactor with it, but it isn’t necessary as often. The TDD forces me to say “what do I want out of this method/class” rather than to say “what should this do and how should it relate to others”. The difference is subtle, but interesting. The TDD way, I’m looking at each method as an atom and each class as a unit, whereas the other way I’m putting the cart a bit before the horse and looking at larger interactions. I thought this was a benefit, but it resulted in more scratch out work during prototyping than was really necessary. I was concerned that TDD would force me to throw away more code because I’d be tossing tests as well as code, but in reality, I just toss less code.

Regarding (2), this has sorted itself out as well. TDD does tend to lead you down the happy path as you’re using it to incrementally correct and refine a method’s behavior, but there’s nothing that stops you from tossing in along with that a thought of “how do I want this to handle an exception in a method that it calls?” This, I’m finding is a discipline and one that I luckily brought with me from testing after or testing during. It is not incompatible with TDD, though TDD does help me not get carried away focusing on corner cases (which can be ferreted out with tools like Pex or smoke tests later).

Regarding (4), this has also proved not to be too much of an issue, though I have seen a touch of this. But, the issue really comes in when it turns out that I don’t need some class, rather than a swath of tests. TDD helps my classes be better thought out and more decoupled, and so, I’ve found that when I’m cutting things, it tends to be surgical rather than a bloodbath.

Regarding (5), this is true, but I’ve fought through the burn. However, there is another thing that goes on here that I’d point out to people considering a foray into TDD. Imagine the scenario that I described above, and that you’re the non-TDD hare and I’m the TDD tortoise. I’m plodding along, red-green-refactoring my parser when you’ve finished yours. You’re probably running it on an actual file and making changes while I’m still working methodically. By the time I’m finished with my parser and doing the formatting, you’ve got your formatting finished and are working on the database. By the time I get to the database, you’ve already got things working 90%, and are busy stepping through the debugger and cursing the OleDbCommand API.

But, that’s where things start to get interesting. Often times, in my experience, this is the part of development that has you at the office late, pulling your hair out. The application is 90% done at 4:00, and you’re happy because you just need to make a minor tweak. Suddenly, it’s 8:00 and you’re hungry and cranky and cursing because no one is left in the office. That’s a brutal 4 hours specifically because you imagined in to be 20 minutes at 4:00. But, let’s say you finally get it and go home. I’ll finish an hour or so after you and you win the first leg of the race, though I’ll do it without a lot of fanfare and frustration, since I’m never hitting snags like that with my methodical process.

But now, an interesting thing happens. We send our versions out for Beta testing, and the issues start coming back. You have your own knowledge of your code to go on as you fix defects and perform minor refactorings, while I have 100% code coverage. I quickly find issues, fix them, see all green and am ready to deliver. You quickly find issues, fix them, and then run the application a bunch of times in different scenarios trying to ferret out anything that you could possibly have broken. I’m gaining on you fast. And, I’m going to keep gaining on you as the software grows. If Beta lasts more than a short time, I’ll win. If there are multiple versions of the software, I’ll win easily. At some point, I’ll lap you as you spend way more time repairing defects you’re introducing with each change than fixing the one you set out to fix. The time investment isn’t new TDD versus seasoned TDD. It’s a little time up front for a lot of time later.

Eating Crow

I had tried TDD in the past, but I think I wasn’t ready or doing it right. Now, I’m convinced that it’s definitely the path for me. Somewhere, I saw a presentation recently that mentioned sort of an arc of unit testing. People tend to start out with test-last, and then move to test during, then test-first and then test-driven, according to this presentation (I wish I remember whose it was so I could link and give credit). I think my previous issue was that I had short circuited the arc and was trying to assimilate too much all at once. If you’re a developer trying to convince others of the merits of unit tests, TDD might be a lot to spring on them up front.

I was thinking about why that might be, and something occurred to me from way back in my undergrad days. The first thing we all tend to do is want to write all of the code at once. Since we’re given small assignments to start, this works. It’s hard to turn bubble sort into a lot of different feedback passes — you write the whole thing and then debug the ‘finished’ product. As classes get harder, you try to keep this dream alive, but at some point you have the “aha” moment that you need to get shorter feedback cycles to prevent assembling the whole thing and getting it all wrong. So, you do this by making your code modular and developing components. That carries you through into the early part of your career, and you have a natural tendency to want to get code up and running as quickly as possible for feedback. Then, TDD comes along and changes the nature of the feedback altogether. You’re no longer getting the feedback from your running application, but from your unit tests of small components. So, if you’re building a house app, you don’t start with the House class and run it. Instead, you start with a “Bedroom” class, or even a “Bed” class. While your counterparts are adding rooms to their running houses, you’re getting your “Bed” class working flawlessly. And, while they’re cobbling furniture into their rooms, you’re assembling rooms from their furniture. When they’re debugging, you’re finally ready for your first “F5” feedback. But, you haven’t come full circle and turned into that freshman with bubble sort – you’ve gotten real feedback every 30 seconds or so. You’ve gotten so much feedback, that there’s little anticipation with the first real run of the component you’re working on for that week. You’re pretty sure it’s going to work and you’re going to be heading home at 5:00.

So, yeah, I guess I’m a TDD convert, and will eat some crow for my earlier posts.