API Lost Opportunity: A Case Study
I’m aware that recently there’s been some brouhaha over criticism/trashing of open-source code and the Software Craftsmanship movement, and I’d like to continue my longstanding tradition of learning what I can from things without involving myself in any drama. Toward that end, I’d like to critique without criticizing, or, to put it another way, I’d like to couch some issues I have with an API as things that could be better rather than going on some kind of rant about what I don’t like. I’m hoping that I can turn the mild frustration I’m having as I use an API into being a better API writer myself. Perhaps you can pick up a few tidbits from this as well.
Recently I’ve been writing some code that consumes the Microsoft CRM API using its SDK. To start off with the good, I’d say that API is fairly powerful and flexible–it gives you access to the guts of what’s in the CRM system without making you write SQL or do any other specific persistence modeling. Another impressive facet is that it can handle the addition of arbitrary schema to the database without any recompiling of client code. This is done, in essence, by using generalized table-like objects in memory to model actual tables. Properties are handled as a collection of key-value string pairs, effectively implementing a dynamic-typing-like scheme. A final piece of niceness is that the API revolves around a service interface which makes it a lot nicer than many things for mocking when you test.
(If you so choose, you can use a code generator to hit your CRM install and spit out strongly-typed objects, but this is an extra feature about which it’s not trivial to find examples or information. It also spits out a file that’s a whopping 100,000+ lines, so caveat emptor.)
But that flexibility comes with a price, and that price is a rather cumbersome API. Here are a couple of places where I believe the API fails to some degree or another and where I think improvement would be possible, even as the flexible scheme and other good points are preserved.
Upside-Down Polymorphism
The API that I’m working with resides around a series of calls with the following basic paradigm:
VeryAbstractBaseRequest request = new SomeActualConcreteRequest();
VeryAbstractBaseResponse response = service.Execute(request);
There is a service that accepts very generic requests as arguments and returns very generic responses as return values. The first component of this is nice (although it makes argument capture during mocking suck, but that’s hardly a serious gripe). You can execute any number of queries and presumably even create your own, provided they supply what the base wants and provided there isn’t some giant switch statement on the other side downcasting these queries (and I think there might be, based on the design).
The second component of the paradigm is, frankly, just awful. The “VeryAbstractBaseResponse” provides absolutely no useful functionality, so you have to downcast it to something actually useful in order to evaluate the response. This is not big-boy polymorphism, and it’s not even adequate basic design. In order to get polymorphism right, the base type would need to be loaded up with methods so that you never need to downcast and can still get all of the child functionality out of it. In order not to foist a bad design on clients, it has to dispense with needing to base your response type on the request type. Here’s what I mean:
VeryAbstractBaseRequest request = new GetSomeCookiesRequest();
VeryAbstractBaseResponse response = service.Execute(request);
var meaningfulResponse = (GetSomeCookiesResponse)response; //Succeeds only because I have inappropriate knowledge of an implied coupling
var secondmeaningfulResponse = (GetSomeBrowniesResponse)response; //Bombs out because apparently brownies and cookies aren't fungible
var thirdMeaningfulResponse = (GetSomeBrownieBytesResponse)response; //Succeeds oddly because apparently BrownieBytes are children of Coookies (or a conversion operator exists)
Think about what this means. In order to get a meaningful response, I need to keep track of the type of request that I created. Then I have to have a knowledge of what responses match what requests on their side, which is perhaps available in some horrible document somewhere or perhaps is only knowable through trial/error/guess. This system is a missed opportunity for polymorphism even as it pretends otherwise. Polymorphism means that I can ask for something and not care about exactly what kind of something I get back. This scheme perverts that–I ask for something, and not only must I care what kind of something it is, but I also have to guess wildly or fail with runtime exceptions.
The lesson?
Do consumers of your API a favor. Be as general as possible when accepting arguments and as specific as possible when returning them. Think of it this way to help remember. If you have a method that takes type “object” and can actually do something meaningful with it, that method is going to be incredibly useful to anyone and everyone. If you have a method that returns type object, it’s going to be pretty much useless to anyone who doesn’t know how the method works internally. Why? Because they’re going to have to have an inappropriate knowledge of your workings in order to know what this thing is they’re getting back, cast it appropriately, and use it.
Hierarchical Data Transfer Objects
With this interface, the return value polymorphism issues might be livable if not for the fact that these request/response objects are also reminiscent of the worst in OOP, reminiscent of late ’90s, nesting-happy Java. What does a response consist of? Well, a response has a collection of entities and a collection of results, so it’s not immediately clear which path to travel, but if you guess right and query for entities, you’re just getting started. You see, entities isn’t an enumeration–it’s something called “EntityCollection” which brings just about nothing to the table except to provide the name of the entities. Ha ha–you probably wanted the actual entities, you fool. Well, you have to call response.EntityCollection.Entities to get that. And that’s an enumeration, but buried under some custom type called DataCollection whose purpose isn’t immediately clear, but it does implemented IEnumerable so now you can get at your entity with Linq. So, this gets you an entity:
RetrieveMultipleResponse response = new RetrieveMultipleResponse();
Entity blah = response.EntityCollection.Entities.First();
But you’re quite naieve if you thought that would get you anything useful. If you want to know about a property on the entity, you need to look in its Attributes collection which, in a bit of deja vu, is an AttributeCollection. Luckily, this one doesn’t make you pick through it because it inherits from this DataCollection thing, meaning that you can actually get a value by doing this:
RetrieveMultipleResponse response = new RetrieveMultipleResponse();
object blah = response.EntityCollection.Entities.First().Attributes["nameOfMyAttribute"];
D’oh! It’s an object. So after getting your value from the end of that train wreck, you still have to cast it.
The lesson?
Don’t force Law of Demeter violations on your clients. Provide us with a way of getting what we want without picking our way through some throwback, soul-crushing object hierarchy. Would you mail someone a package containing a series of Russian nesting dolls that, when you got to the middle, had only a key to a locker somewhere that the person would have to call and ask you about? The answer is, “only if you hated that person.” So please, don’t do this to consumers of your API. We’re human beings!
Unit Tests and Writing APIs
Not to bang the drum incessantly, but I can’t help but speculate that the authors of this API do not write tests, much less practice TDD. A lot of the pain that clients will endure is brought out immediately when you start trying to test against this API. The tests are quickly littered with casts, as operators, and setup ceremony for building objects with a nesting structure four or five references deep. If I’m writing code, that sort of pain in tests is a signal to me that my design is starting to suck and that I should think of my clients.
I’ll close by saying that it’s all well and good to remember the lessons of this post and probably dozens of others besides, but there’s no better way to evaluate your API than to become a client, which is best done with unit tests. On a micro-level, you’re dog-fooding when you do that. It then becomes easy to figure out if you’re inflicting pain on others, since you’ll be the first to feel it.
I’ll do anything in my power to help you get the phrase “big boy polymorphism” to stick.
Let’s do it! 🙂