What to do if a core function does exactly what you need to do, but has a bug

Question

This is something that I come across fairly often, and there are a couple ways to proceed, but never know which is the best way. I usually pick one way at random. I'm looking for a framework that I can apply each time I am faced with this problem.

The problem is this:

I am working on a bug. In this case, the bug is due to assuming user input is in a certain format, but it can come as a different format. We come up with the solution to normalize input to the expected format.

I find a function which does exactly this. Its description explicitly says that it normalizes input to our desired format. However it looks like it has a bug and the output value is invalid. This function is used in quite a few places in our application. I don't see how this could be working correctly everywhere it is used. I think because the bug is "invisible". It doesn't produce a fatal error and only causes a hard-to-detect visual bug.

Another kicker is that this function has test coverage and it explicitly expects the invalid value.

For a practical example:
Let's say the function normalizes a CSS color value to hex color values, but neglects to prepend the # sign in front. Maybe something like this:

function normalizeCssColor(color) {
    // ... logic here which parses the value and gets the rgb values in hex

    // note it doesn't prepend the color with #
    return red + green + blue
}

Note this is just a trivial example, the real code is slightly more complex.

At this point I have a couple options:

1. Directly update the method, then test everywhere it is used. This isn't really possible because the bug ticket wasn't estimated to take all this testing into account.
2. Add a boolean flag to the existing function, which defaults to false, and pass true in the place where I am going to call the function. Like this:

   function normalizeCssColor(color, bool addHash = false) {
       // ... logic here which parses the value and gets the rgb values in hex
       if (addHash) {
           return '#' + red + green + blue
       }
       return red + green + blue
   }
   

   With this approach, we can also investigate later and see if this boolean is needed, and make prepending the # default behavior.
3. Just create a separate function which does what I need (i.e. return '#' + red + green + blue). And then create a bug ticket to investigate if the other places where the old function is used are truly broken and if the function needs to be updated to call my new function.

Answer 1

I suspect this is a fight over a name.

Option 3 (create a separate function) is absolutely the best. Provided you can think of a good name for the new function. But I suspect you can't. That's why people are suggesting things like normalizeCssColorWithHash. Yuck. I think you'd really prefer that only one function was named normalizeCssColor and it just worked without having to think about stupid hash details. Which is why you're thinking about option 1 (update existing).

A truly good name won't focus on the steps taken inside the function. It will focus on the expectations of the code outside it. Give me a name that explains why you would call it. Not what it does. But such a name isn't always available.

Without a good name, option 1 is worth serious consideration. Nothing messes up a code base worse than bad names. However, option 1 is a lot of work. It explodes the scope of your ticket. It makes option 3 tempting. However, option 3 comes with the risk of never following up and leaving you stuck with a bad name.

Fundamentally, with option 1, you are overturning a convention, "color tags don't have a hash", with another one "yes they do". Spreading a design decision like this around makes me sad but I'll admit there are times when you can't avoid it. The pain you're feeling is about how widely it has already been spread.

When faced with decisions like this I take the cowards way out. I do the quick and expedient thing as fast as I can (option 3) then pretend I didn't and see how far I get with the serious solution (option 1). If I can get option 1 done before the deadline (whatever your sprint is) no one ever knows I had option 3 done already.

If I doubt if can hide the dilemma like that I stop mid sprint and ask the team. Who usually go with the conservative solution (option 3) unless the problem (the name) is really bad.

Of course option 3 (create a separate function) doesn't fix the "hard-to-detect visual bug". If you can't fit fixing that in your sprint then toss it into the backlog and get it prioritized. The most important thing to do here is explain the bug well. The work to do here is hard. The bug is subtle. So people will resist fixing it. Find a way to make the problem glaringly obvious. Write a test if you have to. Don't let them sweep it under the rug without a fight.

Answer 2

Option 4.

There is no bug in the core function.

I find a function which does exactly this. Its description explicitly says that it normalizes input to our desired format.

this function has test coverage and it explicitly expects the invalid value

Both the in-code documentation and the unit test say that this core function operates as required.

While I see that the example scenario is simplified and may benefit from re-writing, I would describe the scenario as "a developer imposing a personal style preference upon the code, and breaking working code". A developer should expect their fellow team members to get very annoyed if this occurs.

Answer 3

Out of your 3 options, I would only reject #2 out front. Adding a flag argument often leaves the complexity in the code and forces future readers to understand both options before choosing one. This can easily grow overly complex.

Directly update the method, then test everywhere it is used.

Best option, but you need to take the time. Ideally, you can spend time on fixing bugs thoroughly before new features are considered.

Just create a separate function which does what I need

Close contender, this is second best, ususally used for large projects. This options allows you to incrementally change the code base. You can phrase the second function in terms of the first and then search for all usages of the first. Maybe you even find an exception where the # is not needed after all.

You can call it normalizeCssColorWithHash and move forward.

For large projects, it becomes very important to be able to make incremental improvements as all-in-one PR get more difficult.

Answer 4

You really should go with #1. Having a utility that doesn't do what it's documented to do is a ticking time bomb that will only continue to make things harder in the future. What happens if one of the areas where the bug results only in a "minor visual error" changes so that it results in a serious lapse instead? Be glad you found the issue now without that happening.

#2 is a bandaid approach. Functions should have one and only one... function. Adding flags to make them perform different functions is a code smell that makes your code harder to read, harder to document, and harder to use.

An example: You're reading through the code and run across the expressions square(7.5, true) and square(7.5, false). What's the difference? Whoops, you need to go look at the documentation to find out, and it turns out that with false it rounds off to an integer because nobody bothered to make it work for floats at first and somebody just added a flag for whether or not it should handle floats correctly. This is no way to manage a code base.

#3 is a code duplication issue and can be also confusing to use. This is what leads to things like PHP's notorious mysqli::real_escape_string function. Meanwhile, the incorrect mysqli::escape_string stuck around for years, and everybody who used it opened a vulnerability in their application until they finally just changed it to behave the same as mysqli::real_escape_string.

Consider again our example: This time, you come across the expressions squareFloat(7.5) and square(7.5). Just as in the last case, you're stuck running to the documentation trying to figure out what the difference is. Same thing: the person who implemented square didn't make it work for floats, so some other enterprising programmer came along and added squareFloat.

But guess what? People looking for a squaring function will still find square first because that's the name they expect to find, and they will give it 7.5 and wonder why it doesn't work (assuming they actually notice). This is arguably worse than #2 in this regard, because at least the presence of a flag parameter will alert future developers to the fact that they have a choice to make, whereas this approach does not.

Answer 5

I would suggest that quite possibly what you have here is down to the poor naming of the code that exists.

The method name normalizeCssColor doesn't really mean that much to me

• What are we normalising from?
• What is the output going to be?

In short, I would be uncertain from reading this name what the method is really for.

It's also plainly been set up to work in this way and something else in your codebase is dealing with pre-pending a hash when required.

I would suggest that you keep this method as is, but write another one that wraps around it to do what you need, and name this one better, e.g.

function toHexColorCode(color){
    return "#" + normaliseCssColor(color);
}

You haven't stated what type color is, but possibly having this as an extension method might also make sense.

With the current defect fixed, I would suggest creating a further tech debt ticket to then assess whether the current usages of normaliseCssColor could then be migrated to your new method or not.

Open up team discussion

Creating a separate ticket gives a chance for team to chip in - maybe there is a reason why this was left this way.

• Will changing this require changes in BobsNightmareHexParser code.
• Are some of these values being normalised before persisting to a DB and then used. Changing this may require a migration script.

This allows you to utilise the collective memory of the team and get a decent assessment of risk/reward for this change before just dipping straight into it.

Eases load on reviewers

Assuming you are having your code reviewed, this eases the cognitive load on your reviewers by giving them two distinct changesets:

• One changeset that fixes the defect you were assigned. This will be relatively small and targetted.
• One changeset that is just refactoring - i.e. relatively mechanical and hopefully identical changes in multiple files. Again, much easier to review than mixing the initial bug fix and refactor together.

Answer 6

Which approach to take depends heavily on historical context.

There may be obscure reasons why the function you are looking to use was designed the way it is now, and it might be on purpose that it doesn't do what you expect. Having a look at the history of the code in your versioning system, going through related bug report to understand the changes and asking someone who was there at the time working in that area might help you understand which one is of the three.

I would use #1 if I have verified that the output you expect is indeed the one other people expect as well and it is indeed a bug. In that case you might want to create a new bug to account for the additional effort.

#2 and #3 might be used interchangeably when the "wrong" output should be retained to avoid possible regression. #3 creates more duplication, #2 more spaghetti code. Pick your poison.

Answer 7

Another kicker is that this function has test coverage and it explicitly expects the invalid value.

In the specific case of test coverage and explicit testing for "invalid" results, there are two possibilities:

• The docs are wrong, and this is in fact the intended behavior of the function.
• The tests were simply written to reflect the implemented behavior of the function:
  • Been there, done that: You implement the function, and then write some tests and tweak the result checks until they are green.
  • You then call the function from production code and see if it "behaves".

Without looking at real code, you cannot tell what the best course of action is.

I would, however, say that -- again depending on the quality of the code base -- the mere existence of tests "verifying" "bad" behavior is not on it's own a reason to forego changing it.

Sometimes the tests are as crappy as the implementation, and you just have to fix them both.

Answer 8

I see several answers here focusing too much on the specific example, which I expect is as much contrived as actual.

Within this contrived example, I believe you would be wrong to classify this as a bug. The function does not meet your expectations, but you are one person and your expectation does not define the world. The function seems to be running effectively elsewhere in the application with few to no reported issues. That's not a bug; that's you wishing things were different than they are. Wishful thinking is not reality.

In this context, none of the three proposals is good, because you are only one member of a team. This is something for the team to decide if its worth the time and risk to change.

Given this, the most direct action I could suggest is to file a bug against the unit test that allows the function to pass.

Filing a bug, rather than correcting the test directly, places the issue in front of the appropriate people, where a decision can be made to either really "fix" this (if it is broken) or close the bug as wontfix or bydesign.

From there, correcting the test bug will naturally cause the method to be fixed, and correcting the method will then cascade appropriately to other places. Assuming adequate test coverage, any issues will trigger new failures which will automatically be in scope for correction.

But again: contrived example. We need a general answer. However, the general answer is still to file a bug report, so the issue can be raised and discussed at the appropriate levels.

Answer 9

If you change the function, you will probably find that other people who call the function are prepending their own hash to the output. So it will stop working for them.

What I would do is (i) write my own function that calls the buggy function and then prepends a hash to the result only if no hash is present; and (ii) report the bug in the appropriate place.

Answer 10

Great lesson in why not to have helper libraries.

Basically it's about how pragmatic you want to be. Do you spend lots of time verifying the correct functionality and correcting accordingly. Or do you let sleeping dogs lie and write a new function?

If you are asking on a forum about best practice, obviously we are going to say #1 or, move the code to a versioned library and consume via a package manager! or "duplicate question!" etc

None of the other ideas are great, adding the extra param confuses the matter more, a new function is good, but then you are adding duplicate code all over the place and so on. Doesn't mean I wouldn't do them in a pinch.