Can you consider a bug fixed when the underlying cause was not fixed?

Question

Let's say there is an extensible system in place, which uses an algorithm that requires that the elements it operates on are unique.

The signature to call this algorithm looks like this: function algorithm([Collection of Elements])

Now there are multiple modules that use this algorithm, which call the algorithm like this:

[Collection of Elements] elements = [...]
[make sure the Collection only has unique elements]
algorithm(elements)

Say, there is a new module added which looks like this:

[Collection of Elements] elements = [...]
algorithm(elements)

(i.e. it doesn't make sure that elements are unique).

Now, in a majority of cases all elements are unique. But once in a while an Exception is raised by the algorithm complaining about the duplicate elements.

The module should should have only given unique elements to the algorithm, and as such the exception at that point is reported as a bug and should be fixed.

Now, would the bug be "fixed" by simply adding [make sure the Collection only has unique elements] to the offending module [Fix #1]? Or would the bug be fixed by changing the signature of the algorithm to function algorithm([Set of Elements]), so that the bug that's fixed is impossible to reoccur with any future modules [Fix #2]?

I'm specifically asking about this coming up in a review - should the bug fix be "done" if you review Fix #1? Should the changes of Fix #2 be added as a new refactor task? Should Fix #1 be rejected until it is implemented like Fix #2?

Answer 1

Illegal arguments come it great variety: the range is wider than what our programming languages' type systems can generally express. Our type systems are not general purpose constraint languages, and further, many conditions we might want to check for are dynamic, so must be checked at runtime.

Thus, in general, we still must check for and handle bad input, and, when the functions are properly documented, passing an illegal argument is the fault of the caller rather than the callee and throwing an exception is reasonable behavior.

However, that being said, fault does not necessarily determine how such issue is addressed, as it is reasonable from a technical perspective to fix either:

• the caller — to do what it takes to generate the proper, legal argument, or,
• the callee — to widen the interface, e.g. to allow and handle the (formerly illegal) argument

Your thought of changing the interface to use the type system to ensure the proper argument is super valid (especially in this case, but in general case of illegal arguments, not always possible to do). However, broadly speaking, that change will require making an engineering trade off in maintenance (and possibly performance) about changing all the callers, and against other possible approaches.

If most the callers are doing this,

[Collection of Elements] elements = [...]
[make sure the Collection only has unique elements]
algorithm(elements)

Then you perhaps should provide a version of the interface to the algorithm that does the intermediate step (make sure...) for them. This not only addresses the issue of (future) callers failing to honor the contract, but makes the code base more DRY.

To add to that, the algorithm might offer an interface having a number methods, with varying performance traded off against tolerance of argument. The version above that allows duplicates, and the version that doesn't (is well documented, and throws on violation) and let callers do what is natural for them. You might also include the Set version and see who uses it. It doesn't seem to me like a huge burden for the packaging of the algorithm to provide several such methods.

---

Can you consider a bug fixed when the underlying cause was not fixed?

Yes, if the immediate bug is fixed.

Engineering involves making trade-offs, some of which favor the short term situation and realities at the risk of technical debt that increases the burden of maintenance, cost of change, and correctness.

However, in part this goes to the notion of what the underlying cause actually is, which can be subjective. Was the underlying cause failure to create proper documentation? Failure to read/use the documentation? Failure to keep the code DRY? Failure to design a foolproof API?

You are right, of course, that whenever possible we should foolproof so the other programmers consuming our APIs (often they are us) fall into the "pit of success".

The module should should have only given unique elements to the algorithm, and as such the exception at that point is reported as a bug and should be fixed.

Should Fix #1 be rejected until it is implemented like Fix #2?

No, not necessarily. Fix #1 alone is a quick fix that accumulates a few grains of technical debt, but it addresses the bug in a valid way.

(To be clear I think there are better fixes, that are also cheap to implement.)

This short term decision (fix #1) can be partially mitigated, for example, by beefing up the documentation as this can usually be done at little expense. You should also code review (and/or test) other callers. Further, you can also add to your backlog a technical-debt refactor item to determine the costs (time & performance impact) to clean this up.

Answer 2

Both are fixes to the bug; they both remove or prevent the situation causing the problem from occurring again.

Fix #1 is possibly a bit short-sighted, as there may very well be another situation where a new module is added and the developer forgets / doesn't know about the check for uniqueness. Fix #2 solves that potential future problem by making it impossible for the elements to not be unique, but comes at a cost of refactoring the existing modules to accept the new signature. The amount of refactoring may be small or it may be large, we don't know (but hopefully you at least have an idea), and that may dictate whether you want to implement fix #1 or #2.

Answer 3

They're both "fixes" for the bug, but depending on how the uniqueness is enforced, you might be asking for trouble:

function algorithm(const UniqueList *list)

will force all well-behaved methods to change their signature to match. On the other hand,

/* Hey, list must be unique! */
function algorithm(const List *list) {
    ...
}

can be seen as lawyering. Even a "helpful" refactoring like

/* list is made unique if it isn't */
function algorithm(List *list) {
}

can be deadly if any of the callers expects its list to not change, or wants to be responsible for its memory management while the function might free non-unique elements and so on.

The way I'd try to go is:

• deprecate the old function, replacing it with one that checks and raises the appropriate hell
• supply two interfaces for both use cases.

Then the users will decide whether to do nothing (they can), or use whichever function best suits their situation. The code you really need to add is very few lines and some casts (in my experience, several users will start gratefully dropping their uniqueizing code to rely on yours).

/**
 * Old function
 * WARNING: list must be unique
 * @deprecated code has been moved to algorithmU
 * @see algorithmU
 * @see algorithmN
 */
function algorithm(const List *listThatMustBeUnique) {
    assert(isUnique(listThatMustBeUnique);
    return algorithmU((const UniqueList *)listThatMustBeUnique);
}

/**
 * Applies algorithm to a unique list of elements.
 */
function algorithmU(const UniqueList *list)

/**
 * Generalized interface to algorithm.
 *
 * @see algorithmU
 */
function algorithmN(List *list) {
    // makeUnique comes from the best implementation of "ensure list is unique" seen around.
    UniqueList *list2 = makeUnique(list);
    return algorithmU(list2);
}