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.

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.