I shared this note internally in Myntra in 2015 when I was working on some particular feature of their Order Management System (OMS). Sharing it now with internal details redacted.
I broke OMS yesterday. Didn’t think I would, but still did. And I am mad as hell about it.
tl;dr — Always be explicit about the inputs when exposing APIs. Relying implicitly on the caller or some other global condition is a horrible horrible idea.
OMS is a large project. We maintain the entire life cycle for Myntra’s orders, and shit ain’t easy. The challenge, however, is not in building the individual sub-parts, but in building the flow.
There are two ways to approach this. One is to build tightly coupled parts which only make sense as a whole. This approach chooses maintainability to the detriment of flexibility and reuse. Essentially, the whole can be implemented as a single piece (in the logical extreme, a sequence of method calls invoked from one place). The internal parts rely heavily on where they are located in the workflow and each part sets the context for its successors.
The other approach is to build an overall scaffolding and reusable pieces which can be plugged in as needed. This approach makes the reverse trade-off. While writing a component, we never know what is happening around us and so we have to code in an isolated manner. This means tremendous flexibility, but may mean that the overall flow isn’t optimized (Read from DB too many times, write to DB too many times to manage state in isolation).
Over its years, OMS has evolved (decayed???) into a mix of both of the above. We have a lot of semi-independent components which work alone. Some of them need loving from their neighbours and some don’t. Of these, there are some psychos that pine away internally but wont say that all they need is love.
What is wrong with this API: CancellationUtil.issueCancellationRefund(Order order);On the face of it, nothing. Looks intuitive. Reads like a story (issue-refund-for-the-order-object-passed-to-you). A good API.
Except that it isn’t. The actual implementation of the API assumes that certain fields of the input object have been manipulated in a certain way (the caller should take the applied coupon code from order and set it into all order items). The problem is not that it needs such a manipulation. The problem is that the API doesn’t make it obvious that it relies on some very specific pre-processing of its inputs. That is to say, it is aware of its location in a workflow. Something like this would be better: CancellationUtil.issueCancellationRefund(Order order, String appliedCoupon);
Or even better:
CancellationUtil.issueCancellationRefund(int orderNumber);// Everything is done inside the API.
At long last, we come to the point of this rant. IMPLICIT EXPECTATION IS THE ENEMY. You know you are in a bad place when absolutely unrelated changes start breaking you. This is why our “trivial” changes sometimes break catastrophically. This is why we are afraid of refactoring (I certainly am!). This is why we prefer rewrites to maintenance.
Let’s face facts. Writing and maintaining great documentation is hard :). But what we can and must do, ALL THE TIME, is look at our code real hard, and remove any hints of subtle, hidden agreements from its interfaces. Bad looking APIs are ok. Bad performing APIs are ok (performance can probably catch up), but that gentlemen’s agreement between caller and callee isn’t going to honour itself.
Read Next : Extending code vis the Resource Locator pattern
