23

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.
18
  • 13
    What makes you think that any one of these approaches is "the right answer"? Isn't it much more likely that sometimes you do one of them and sometimes you do another depending on the specifics of the situation (impact, timescales, work required, etc, etc, etc)? Jun 19, 2023 at 20:23
  • 81
    Your bug tickets have estimates? Honestly? Do the persons who file a bug have a crystal bowl where they know the root cause of the bug before it is analyzed? I mean, you may not have enough time for a certain approach, but usually for other reasons.
    – Doc Brown
    Jun 19, 2023 at 20:55
  • 57
    This isn't really possible because the bug ticket wasn't estimated to take all this testing into account. Sometimes (most times?) estimates are wrong...
    – mmathis
    Jun 20, 2023 at 0:30
  • 5
    If the test documents that the function does not do what you want it to do, this is not the function you want to use. Find another or write a new, correct one. Jun 20, 2023 at 5:26
  • 22
    This isn't really possible because the bug ticket wasn't estimated to take all this testing into account. -- You don't have a programming problem, you have a business problem. Jun 20, 2023 at 14:46

10 Answers 10

34

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.

19
  • 22
    If we have the codebase fully under control and good refactoring tools, we can also rename normalizeCssColor to normalizeCssColorWithoutHash first (including all calls to that function) and afterwards introduce the new function as normalizeCssColor, That makes it more obvious during the following investigation that we suspect the "old" function to behave in a potentially unwanted manner.
    – Doc Brown
    Jun 20, 2023 at 5:44
  • 6
    @DocBrown In large code bases with lots of developers working at the same time that can be dangerous though, because other concurrent branches could inadvertently end up calling the wrong function (no merge conflict and compiles). More of a danger for shared helper utilities than other code.
    – Voo
    Jun 20, 2023 at 10:23
  • 9
    @DocBrown I like it. Stick the bad functions with the bad names and they can die together. Jun 20, 2023 at 13:02
  • 3
    @Davor It is a rather subtle problem and one that only appears in large code bases with lots of concurrent dev on longer-living branches. Rather insidious and many people don't know about it, which is why I wanted to mention it. Here's a gist that demonstrates the problem.
    – Voo
    Jun 21, 2023 at 12:49
  • 2
    @Davor One of the two hard problems next to cache invalidation and off-by-one errors ;-)
    – Voo
    Jun 21, 2023 at 13:07
32

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.

3
  • 1
    Yea this was my first reaction as well. If you don't understand why something works, then you are not supposed to make changes about it. One way forward is to investigate why it works. But if it's not an option, then Option 1 is also out. So only Option 3 remains (why Option 2 is bad is already covered by other answers). It'll look weird (one with hash, and one without), but that's the best OP can do given that OP doesn't have yet full understanding on how the function worked downstream in the first place.
    – justhalf
    Jun 21, 2023 at 21:12
  • 3
    Generally, while I fully support deferring to the team, I firmly stand opposed to deferring to existing code just because it got there first. That's just lazy inertia. Fight it with every spare cycle you can or soon you will have no spare cycles. Make the code base better. Now specifically, the key words being forgotten here are "causes a hard-to-detect visual bug". So you're arguing in favor of broken code. Jun 22, 2023 at 3:11
  • @candied_orange: Adding a new function which behaves in a more useful fashion seems a much better course of action than changing how an existing function will behave in scenarios that are likely to arise, and upon which programs are likely to rely. Even if the existing function were deprecated, giving new functions names which match those of previous functions that behaved differently is a recipe for disaster.
    – supercat
    Jun 22, 2023 at 19:59
9

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.

9
  • 3
    I agree mostly, but sometimes the less risky and most DRY variant of creating a new separate function is to add an optional boolean flag to the old function (maybe temporarily) and implement the new separate function by calling the old function with the boolean flag set accordingly. Then, when the code works as intended and maybe got some additional tests, one may refactor and try to get the boolean flag out of the code again.
    – Doc Brown
    Jun 19, 2023 at 20:54
  • 3
    @Bergi Why is it a risk that someone call the function and pass the flag? In your example with additional abstraction there is also a risk that they call the wrong function. If there is a logically named parameter and someone uses it wrong, that's not really something you can mitigate, that's on the user of the function.
    – rooby
    Jun 20, 2023 at 5:27
  • 1
    @rooby I meant in particular someone passing a dynamic value for the flag. This defeats the goal of incrementally migrating the code base to the new function, and rather introduces code that makes the flag-taking function harder to refactor. It goes against the general wisdom to avoid flag parameters (see e.g. here but also elsewhere). As Doc Brown said, it can be more DRY on the short run, and it's ok to do this temporarily until the migration is complete, but it should be restricted in scope.
    – Bergi
    Jun 20, 2023 at 5:36
  • 2
    @Bergi I'm familiar with Martin Fowler's opinion on flag arguments, but I don't consider it a "you should always do this" thing, but more of a "be aware of the possible downsides of doing this" thing (I guess that's the case for most things). It can also happen that going too far down the route of abstracting everything into separate functions can turn into a mess that is harder to work with than it should be.
    – rooby
    Jun 20, 2023 at 6:37
  • 1
    ...then it might make sense to apply the workaround and use it unless or until application needs change, but make clear that if that happens future maintainers should feel free to replace the quirky function rather than adding more layers of workarounds.
    – supercat
    Jun 22, 2023 at 20:11
6

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.

5
  • You are disregarding the unit test. Someone looked at this function saw that it performed in a manner that OP considers "wrong", and decided that this is intended behaviour.
    – Taemyr
    Jun 22, 2023 at 11:14
  • I am not disregarding the unit test. Unit tests can be wrong too. Many a developer has written unit tests based on the assumption that the existing behavior is correct. Is that useful? No, but people do it nonetheless. If we accepted unit tests as gospel, then we'd never refactor either. The solution is never to make a "new and improved" version next to the old version, nor to put a flag argument. Jun 22, 2023 at 13:06
  • Sure,unit tests can be wrong. However even when wrong they represent a past decision that the tested behavior is intended. And discarding that decision is beyond the scope of fixing a bug that occurs elsewhere in the solution. If the situation is as decribed in OP I would solve the bug by; Updating or writing a method documentation to make it clear that it does not include the hash symbol; Update the bugged method to use the old function - Ie. var color= "#"+oldfunction([args]); creating a ticket for changing the old function with a description to indicate flaws in current solution.
    – Taemyr
    Jun 22, 2023 at 23:54
  • @Taemyr And by writing "#" + oldfunction(...), OP would be adding more tech debt that will have to be addressed when the existing broken function is fixed. Depending on the practice of OP's team, OP may want to create another ticket that blocks the original, but the pros and cons of that decision would be better debated at PM.SE. Jun 23, 2023 at 12:53
  • Yup, using the old function adds technical debt.
    – Taemyr
    Jun 26, 2023 at 13:01
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.
2

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.

3
  • How does #2 create spaghetti code?
    – JimmyJames
    Jun 20, 2023 at 15:28
  • 1
    @JimmyJames it usually leads to adding parameters and flags to functions to allow to retain the old behaviour where necessary while exposing the new one where required. A simple case like that is fine, but take a more complex function and do that a couple of times with a two or three flags and the result will easily not look nice :)
    – bracco23
    Jun 20, 2023 at 16:05
  • If you're creating more duplication, you're doing #3 wrong.
    – Ben Voigt
    Jun 20, 2023 at 16:12
2

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.

2
  • 1
    The unit test is a strong signal that the team is aware of the behavior. It is extremely unlikely that changing a method from returning "010203" to "#010203" is not going to break stuff.
    – Taemyr
    Jun 22, 2023 at 11:16
  • @Taemyr - good for you, if, in your team, the Unit Test is any signal at all. In my team, the unit test is no signal. It was written, and if we're lucky it was reviewed. And that's it. So I guess it is dev and team and company specific if you can rely on the specifics of a UT.
    – Martin Ba
    Jun 24, 2023 at 11:25
2

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.

3
  • It's definitely a bug. Otherwise, there wouldn't be a ticket to fix it. That said, the crux of your answer is absolutely correct. The right answer is to close the current ticket outright as 'won't/can't fix' and simply open a new one that can be correctly estimated. Jun 22, 2023 at 13:33
  • This seems contradictory: first you say "That's not a bug; that's you wishing things were different than they are. Wishful thinking is not reality", and then you suggest to file a bug! What would you write in the bug's description? "I wish the function did a different thing, please change it"? As you said, that's not a bug... Jun 22, 2023 at 16:34
  • It only seems contradictory. "File a bug" is a colloquial term meaning "to file a ticket in the issue tracker". It doesn't mean it is literally a bug. OP could write "This function is specified to have this behaviour, but for these reasons we should change the specs to specify that behaviour instead." I do agree Joel could have been more precise with their language to prevent this confusion.
    – Jasmijn
    Jun 22, 2023 at 20:43
2

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.

0

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.