Is it better to perpetuate a bad (but consistent) pattern OR implement a new solution using an improved pattern?

Typically, I advocate for the latter, with the caveat there should be ongoing communication with product managers and teammates about propagating the new pattern in the rest of the code base through tech debt tickets. The paradigm changes, however, when discussing APIs that are commonly used — rather than general coding practices.

Recently at work, I ran into a situation where this exact question came up. In our case, the specifics were a bit more complex, but the crux of the problem (and the gist of the solutions) remains the same.

The Situation:

1. Let's say we use an extensible Grails service to handle common logic (deletes, lists, etc.) that may be inherited & expanded upon by other services.

2. One of the ways this service is extended is through the use of a map in each domain, allowing that service to interpret params from a user without exposing the actual domain field names.

3. Example:

   • We add a property to a domain class called dateDeleted, which is initialized as null and set to new Date() when a user hits a delete endpoint.

   • This field is mapped like so:

     1def API_PARAM_KEYS = [ deleted: dateDeleted ]

   • Then, when making a list or get call, users see these “deleted” values filtered out:

     1params['deletedExists'] = params['deletedExists'] ?: 'false'

   • If users would like to have deleted values returned, it’s as simple as providing deletedExists: true to the params with their request.

   • The ExtensibleApiService then matches the query with the corresponding column in the API_PARAM_KEYS declaration and applies the filter before querying Postgres.

4. This pattern means anywhere we want to use a dateDeleted field, we end up with functionally three different names that refer to the same thing:

   • The column in Postgres: date_deleted (or dateDeleted pre-Hibernate magic)
   • The corresponding API_PARAM_KEYS value: deleted
   • The param value expected by our list API: deletedExists
   • Note: none of these matches what I would tend to consider the “default” boolean name (isDeleted), so that’s a fun wrinkle.

Considerations:

1. Consistency is high value, even if it seems suboptimal. If there’s an established pattern - and it works - we have to consider the cost in deviating. This is particularly true when there are many external and internal API consumers who might be affected by a possible change.
2. Having three (or four!) different names for the same thing is generally a bad practice. It does not add value to the system, nor does it improve the team’s ability to build and iterate on the code. There is a barrier to entry in understanding what should be a straightforward API design when building new domain classes.
3. When the scope of a change can be limited as a proof of concept, it must be bubbled up to the team writ large. This way, if it’s successful, it might be retrofitted to older APIs to enforce consistency. If the scope can’t be limited, consideration should be given to tackling it with a more all-encompassing method (quarterly tech debt swarm, anyone?).
4. There is good sense in having a common pattern to not expose data-column or domain-property names to API consumers. Our use of API_PARAM_KEYS is a convenient way to hide these property names. Because hiding these names is a relatively common requirement, it is not necessarily unusual or undesirable to have one name for a property in the API request and another name in the DB table or domain.
5. “Helper” derived properties are generally good. If another developer would like to check this deleted status, they should not need to look up the interface/SQL table to see if that date is nullable, or if it’s mandatory for some reason (and instantiated with some default, e.g. “0000-00-00”) in order to know how to test it.

Brainstorming:

To help make sense of all the considerations (which are of very mixed opinions), it can be helpful to brainstorm a few solutions that solve the root issue. Once these basic solutions are defined, we can see how well they stack up against any limitations or requirements spelled out previously.

1. Adding additional custom getters for derived properties (e.g. isDeleted) to do the thinking for the user.
2. Use a custom trait (e.g. Deletable) with the appropriate deleted date property, to be applied to any domain that needs it.
3. Leave it as-is. Yes, this is a real option! Sometimes, things don't have a convenient path to upgrade. In that case, we're better off NOT breaking anything that's currently in production, even if it'll look prettier or perform better.

It's interesting, because to me both option 1 and 2 would work to alleviate the issue. Under the hood, all they're doing is providing a layer of sugar over the existing code. Either option is equally appealing from that perspective, because they solve the core issue of sacrificing one type of consistency for another.

At that point, the discussion becomes one regarding architecture and speed, which means we get to start asking the fun questions. Would this be the first custom trait in the system? How would we map values from a custom getter to the ExtensibleApiService? There are many new considerations — which is why that third option I listed is more and more appealing as we go.

The Outcome

This question doesn't really have a "solution," per se. In our case, I bubbled up the conversation to the engineering team to discuss. In our team tech review, we weighed pros and cons, but ultimately haven't decided to move forward with any changes. As a result, my code is shipping in a way that's consistent with the rest of the application, because that's the least-risk option available.

Regardless which route we took from the brainstorming, it's clear this is a call to guide us toward better documentation. In my mind, this is a perfect example of something that can't be captured with self-documenting code. We need pragmatic standards for how to tackle this sort of API design in the future, to ensure we are nothing if not consistent.

I will provide a follow-up post if we do revisit any of the ideas we threw around. I'm particularly fond of building out a prototype of the Deletable trait, and will ask our product team to give us time to do exploratory programming this month. I've never built a custom trait in Groovy, so am sure the experience will be informative.