views:

73

answers:

6

Imagine i have a function with a bug in it:

Pseudo-code:

void Foo(LPVOID o)
{ 
   //implementation details omitted
}

The problem is the user passed null:

Object bar = null;

...

Foo(bar);

Then the function might crash due to a access violation; but it could also happen to work fine. The bug is that the function should have been checking for the invalid case of passing null, but it just never did. It was never issue because developers were trusted to know what they're doing.

If i now change the function to:

Pseudo-code:

void Foo(LPVOID o)
{
   if (o == null) throw new EArgumentNullException("o");

   //implementation details omitted
}

then people who were happily using the function, and happened to but not get an access violation, now suddenly will begin seeing an EArgumentNullException.

Do i continue to let people using the function improperly, and create a new version of the function? Or do i fix the function to include what it should have originally had?


So now the moral dillema. Do you ever add new sanity checks, safety checks, assertions to exising code? Or do you call the old function abandoned, and have a new one?


Consider a bug so common that Microsoft had to fix it for developers:

 MessageBox(GetDesktopWindow, ...);

You never, ever, ever want to make a window model against the desktop. You'll lock up the system. Do you continue to let developers lock up the user's computer? Or do you change the function to:

 MessageBox(HWND hWndParent, ...)
 {
    if (hWndParent == GetDesktopWindow)
       throw new Exception("hWndParent cannot be the desktop window. Use NULL instead.");

    ...
 }

In reality Microsoft changed the Window Manager to auto-fix the bad parameter:

 MessageBox(HWND hWndParent, ...)
 {
    if (hWndParent == GetDesktopWindow)
       hWndParent = 0;

    ...
 }

In my made up example there is no way to patch the function - if i wasn't given an object, i can't do what i need to do on it.

Do you risk breaking existing code by adding parameter validation? Do you let existing code continue to be wrong, getting incorrect results?

+2  A: 

The problem is that not only are you fixing a bug, but you are changing the semantic signature of the method by introducing an error case.

From a software engineering perspective I would advocate that you try to specify methods as best as possible (for instance using pre and post-conditions) but once the method is out there, specification changes are a no-go (or at least you would have to check all occurrences of the method) and a new method would be better.

Christopher Oezbek
You're advocating that we leave people to use the existing function incorrectly, producing incorrect results? What's the best mechanism to force everyone to abandon the function?
Ian Boyd
@Ian: This is what the obsolescence is for. In C#, it'd be the obsolete attribute.
Jacob G
@Ian: I would advocate it. Probably people have started to assume that your function is essentially concatenating using commas. Since you did not specify exactely what the method was supposed to do, it is very hard to find out whether people abused it in any way. Jacob's suggestion with obsolescence or @deprecated in Java is probably the best way to get rid of all those incorrect uses step by step.
Christopher Oezbek
@Christopher: The function is designed to convert a city and state into a "proper" format. People were supplying invalid parameters (they were not supply a city in the city parameter). i then started adding more rigorous parameter checking. Nobody can argue that they were using the function incorrectly. Should developers not be allowed to add additional parameter validation, and unit tests, to match newly discovered edge cases?
Ian Boyd
@Ian: You are certainly right and allowed to do it. Still, if this is a critical application then adding new exceptions which were not yet in the signature is dangerous. I would much rather go a defensive route (if it occurs many times that `city+state` is passed for `city` then even stripping the state might be a better change than start throwing exception which might explode in the face of the customer)
Christopher Oezbek
+1: VB3 was shipped with a whole raft of bugs that developers soon became dependant on e.g. I - seem to - remember that when using arrow-keys to navigate down a list box that a click even was fired instead of a selection changed event. However so much production code was written around it that it became a "feature". At the time the official Microsoft line in the MSDN was "this behaviour is by design, this design is under review". That phrase has stuck with me since, I often roll it out at parties :p
Binary Worrier
@Christopher: Not every language, in particular the one i'm using, has a mechanism to mark obsolete functions as obsolete. i'm also not comfortable marking a function as obsolete every few months, as i find new ways to improve it. It turns in a continuous cycle of using the new proper functions. If that were the case everyone would probably be using `String.IsNullOrEmpty7()`
Ian Boyd
@Ian: It is a balancing act and you can make all changes which relax the precondition of the function (you allow more inputs) and stricten the postcondition (the output is even more within the specified range). If you know well who is calling a method you can of course change all call-sites. If you do not have @obsolete or else, go with the warning route that somebody else suggested. Good luck in any case!
Christopher Oezbek
A: 

This is where having automated tests [Unit testing, Intergration Testing, automated functional testing] are great, they give you the power to change existing code with confidance.

When making changes like this I would suggest finding all usages and ensuring they are behaving how you belive they should.

I myself would make bug fixes to existing function rather them duplicating them 99% of the time. If it changes behavior alot and there are a lot of calls to this function you need to be very sure of your change.

So go ahead make your change, run your unit tests, then your automated functional tests. Fix any errors and your golden!

David Waters
In my real situation unit testing would not have caught the problem. Because going forward everything works as it should. The problem would have been interacting in a specific way with old data.
Ian Boyd
+1  A: 

I'd keep the old function and simply let it create a warning that notifies you of every (possibly) wrong use and then i'd just kick the developer who used it wrong until he uses it properly.

You cannot catch everything. What if someone wrote "MakeLocation("Ian Boyd", "is stupid");"? Would you create a new function or change the function to catch that? No, you would fire the developer (or at least punish him).

Of course this requires that you document what your function requires as input.

dbemerlin
The function does document its input. "City" and "State". "is stupid" is not a state, nor is "Ian Boyd" a city. And i know that because i'm a human being.
Ian Boyd
Also, what kind of "warning" can be shows to notify people?
Ian Boyd
A: 

If your code has a bug in it you should do what you normally do when any bug is reported. One part of that is assessing the impacts of fixing and of not fixing it. Sometimes the right thing to do with a bug is to not fix it because the behaviour it exposes has become accepted. Sometimes the cost of fixing it, or the iconvenience of releasing a fix outside the normal release cycle, stops you releasing a fixed bug for a while. This isn't a moral dilemma, it's an economic question of costs and benefits. If you are disturbed at the thought of having known bugs in your published code, publish a known-bugs list.

One option none of the other respondents seems to have suggested is to wrap the buggy function in another function which imposes the new behaviour that you require. In the world where functions can run to many lines it is sometimes less likely to introduce new bugs to preserve a 99%-correct piece of code and address the change without modifying existing code. Of course, this is not always possible

High Performance Mark
The additional problem i'm trying to solve is how to notify developers that they are using a function incorrectly. They are passing invalid parameters, which is their fault. If i had a time machine to add all these edge cases that i didn't think of originally - i would.
Ian Boyd
A: 

Two choices:

  • Give the error checking version a new name and deprecate the old version (one version later have it start issuing warnings (compile time if possible, run time if necessary), two versions later remove it).
  • [not always possible] Place the newly introduced error check in such a way that it only triggers if the unmodified version would crash or produce undefined behavior. (So that users who were taking care in their code don't get any unpleasant surprises.)
dmckee
A: 

It entirely depends on you, your codebase, and your users.

If you are Microsoft and you have a bug in your API that is used by millions of devs around the world, then you will probably want to just create a new function and update the docs on the old one. If you can, you would also want to update the compiler to give warnings as well. (Though even then you may be able to change the existing system; remember when MS switched VC to the C++ standard and you had to update all of your #include iostreams and add using stds to get simple, existing console apps working again?)

It basically depends on what the function is. If it is something basic that will have massive ripple effects, then it could break a lot of code. If it is just an ancillary function, then you may as well fix it. Of course if you are Microsoft and your other code depends on a bug in one of your functions, then you probably should fix it since that is just plain embarrassing to keep. If other devs rely on the bug (that you created), then you may have an obligation to the users to not break their code that you caused to be buggy.


If you are a small company or independent developer, then sure, go ahead and fix the function. If you only need to update yourself or a few people on the new usage then fixing it is the best solution, especially since it is not even a big deal because all it really requires is an added note to the docs for the function. eg do not pass NULL or an exception is thrown if hWnd is the desktop, etc.

Another option as a sort of compromise would be to create a wrapper function. You could create a small, inline function that checks the args and then calls the existing function. That way you don’t really have to do much in the short term and eventually when people have moved to the new one, you can deprecate or even remove the old one, moving the code to the new once between the checks.


In most scenarios, it is better to fix a buggy function, particularly if you are merely adding argument checks as opposed to completely changing the behavior of the function. It is not really a good idea to facilitate—read encourage—bad coding just because it would break some existing code (especially if the code is free!) Think about it: if someone is creating a new program, then they can do it right from the start instead of relying on a bug. If they are re-compiling an old program that depends on the bug, then they can just update the code. Again, it depends on how messy and convoluted the code is, how many people are affected, and whether or not they pay you, but it is quite common to have to update old code to for example initialize variables that hand’t been, or check for error codes, etc.

To sum up, in your specific example (given the information provided), you should just fix it.

Synetech inc.