Untangling Knots

tl;dr: **never ship your first implementation of a feature and be prepared to make trade-offs.

I was happily cutting through email when Patricia — one of our Lead Engineers — asked me to take a look at some code she was reviewing.

It was an initial approach to storing the tax number type for our clients. We needed that to comply with a payment gateway requirements (oh, the fun of making business in uncharted territories). The requirements were simple:

• We needed to send the gateway a different code depending on the tax number type, so we had to store the type.

• We needed to be able to display all available types in each country where Cabify operates, so we could build a UI for them.

So the initial implementation added the new property tax_code_name to the Client model (yes, we are mostly using the Active Record pattern). Instead of a string, we were using a custom TaxCodeName type (we use couchrest_model, so every property is typecasted) that we can evolve in the future with richer behavior.

But we had no instance methods… that was disturbing. Moreover, we did use some class methods from the Client.

— Creating these methods on the Client just to call class methods on TaxCodeName doesn’t feel right — Patricia said.

That was totally true. We had an instance as a property, so why not delegate through that?

It turned out that we needed information about the country to make good decisions about the tax code name. We had the country on the Client so it was the easiest way.

Some brainstorming later we had three other possible implementations.

• Putting the country information in our custom type. This was somewhat redundant (we had it on the Client). It also had the risk of it getting out of sync with the country in the Client if the value were updated.

• Reading the Client country from the custom type. This is a useful feature that couchrest_model provides and we already used in other places. The main problem is that if the Client updated their country, we would have an invalid combination of country and tax code name.

• Updating the tax code property to be a composite object with both tax code name and number. The problem was that we were storing it as a String, so we would need to migrate all our data or do some magic to provide backwards compatibility with existing data.

No obvious best option.

We took a step back. Why was the country important? Could we model the dependency on the country in a different way?

The country was used to scope the tax code name (there is no guarantee that a name is globally unique), but the use case at hand only needed to handle one specific gateway on one specific country!

Right there we found an opportunity to simplify.

• We let the code for the gateway handle the transformation from name to id. Since the gateway was already aware of the country, no scoping was needed.

• We removed the custom type… the remaining logic (retrieving tax code names given a country) was moved from the type to a repository-like object. The tax_code_name property was now just a String.

• Still, the solution was not ideal. We store tax codes in several other models and only having the tax code name on one felt weird. But there was a deadline to meet and the solution was good enough given that we would not be making use of that field event if it existed.

A few UI updates and a few validations to ensure that the property was required for a country and rejected everywhere and the feature was ready to ship to the staging environment.

Even if shipping the first version would have worked, together we found a better option: there is more shared knowledge of the feature and we have some ideas to improve it even more, while still managing to make a quick release.

I’d call that a victory 🙂