Danger of using default values in declarations

Working on a large project using C++, recently faced a dangerous things during
debugging. Let’s see in examples. Once there was a method:

Class1::SetEditBox(CString const& value, bool isUppercase = true);

Somewhere it was used differently:

obj1.SetEditBox(_T("Volume"), false);
 ...
 obj2.SetEditBox(_T("SerialNo"));

After a while this declaration was a bit extended:

Class1::SetEditBox(CString const& value, bool isEditable = false, bool isUppercase = true);

New places were using it properly, but not all that old places were changed, so

obj1.SetEditBox(_T("Volume"), false);

became to have isUppercase = true, however, since volume is usually numeric,
this was not detected during “happy way” testing.
Meanwhile system became more complex. Refactored and rewritten, developers were
also changing. Inheritance and other OOP stuff started to using this method:

Class2::SetListEdit(CString const& value, bool isEditable = false, bool isUppercase = true)
{
    // ...
    obj4.SetEditBox(value, isEditable, isUppercase);
    // ...
}

And again time passed, things changed, and so changed method declaration:

Class1::SetEditBox(CString const& value, bool isNumeric = true, bool isEditable = false, bool isUppercase = true);

Now there were even more things to recheck and update, but guess what?
Somebody, who did this change did not care about looking up every place to change
and now it became the bug, which I called “Shifting Boolean”:

Class3::AddListEdit(CString const& value, bool isUppercase = true)
{
    Class2 obj5;
    obj5.SetListEdit(value, isUppercase);
}

So the call to Class3::AddListEdit() and passing only CString value, made
isUppercase default value bacame isEditable in Class2::SetListEdit() and then
to became isNumeric in Class1::SetEditBox().

If there were no default values, all the differences would come out during
compilation, or even earlier, in IDE. However there are a trade-offs: either
you save typing and introduce default values, or make changes more transparent.

In any case, every change that can be interpreted by implicit type conversion
should be carefully examined and all the places should be changed as required.

It took me about 6 hours to refactor all the project code and get rid of this
bug. In my solution I have replaced booleans with a single enum argument, because
all the booleans were tightly coupled, and in the end switch()-ing between
enumerated values was easier than checking all the boolean combinations.

Leave a Reply

Your email address will not be published. Required fields are marked *