Primitive obsession in API design
Recently I encountered a real danger in form of the primitive obsession code smell: https://luzkan.github.io/smells/primitive-obsession.
Using int as function’s return created a confusion about how many many return
values the function may return, and whether or not they could be interpreted as
boolean value. This non-clarity of API created a bug.
Take a look at this function that reports status of a feature after its flag:
int
foobar_get_feature(handle_t *h, unsigned feature)
{
foobar_t *x = foobar_from_handle(h);
if (!foobar_has_feature(h, feature))
return -1;
return !!(x->active_foobar_features & feature);
}
At the first glance, it appears that the function report if some feature is enabled or not. All existing call sites for the function make it appear as if its return value has a boolean meaning:
if (foobar_get_feature(handle, FEATURE_ABC)) {...};
But it is not so! The function can return -1, 0 and 1. The case of -1 means
that the handle does not support the specified feature and because of
that, it is not enabled.
When the integer exit code is reinterpreted as boolean, both -1 and 1 are converted to True
value. The meaning of “feature is not supported and thus not enabled” is
misinterpreted as “feature is enabled”. Luckily, the function is only used in
internal tests, despite being many years old!
In the tests, the handle version always happens to support all the features.
So foobar_has_feature() happen to never return -1.
I was on the verge of adding its fist use in production code, and almost got caught. That is because all existing use cases, its name and this codebase’s general tendency to represent booleans as integers suggested to me that 0 means disabled, 1 means enabled. Only after reading the function’s body could I notice that there is a third alternative.
I see three possible alternatives on how this issue can be solved:
- The worst one: document meaning of return values in a comment. This will not help to dispel
the impression that is returns a boolean if you look at the existing call sites,
even if they get updated to read
if (foobar_get_feature(...) == 1) {...};. - Better one: remove the function from public API, and only keep it in the test where it is currently used.
- Introduce a new function with return type of an enumeration of three values:
{Unsupported, Disabled, Enabled}. This way, its API becomes self-documented. It also becomes harder to implicitly convert the return value to boolean. It also solves the primitive obsession code smell.