function change - is this a poor option

[bob]

Hi,


I'm working on fixing issues/bugs in a legacy codebase.

I'd like to ask experts opinion on a decision I've made regarding a
function. I don't have my Scott Meyers at hand but I'm fairly sure he
mentions this issue.

consider virtual function;

void foo()
{

if (something)
{
do_x();
}
else
{
do_y();
}
}


Now for reasons we don't need to know in this post, do_x() is the
correct behaviour for the scenario I'm debugging. However as it stands
the code always goes to do_y();


I want to change the signature of the function to;

virtual void foo(bool myFlag = false);

I've also updated the other classes that override this virtual function
with the new default argument (no function hiding etc)..

Now in the code path that I'm interested in (to fix this bug), I call
foo(true). I make no changes to any other code, and so myFlag is always
false in that scenario.

The logic of the function now looks like;

void foo(bool myFlag = false)
{

if ((something) || myFlag)
{
do_x();
}
else
{
do_y();
}
}




Now this will work for me. However Scott Meyers (vaguely from memory) I
*think* says this is not a good idea. Would people out there agree? Is
this a recipe for disaster or a sound approach.

thanks much.

Graham

 

[Jim Langston]

Hi,


I'm working on fixing issues/bugs in a legacy codebase.

I'd like to ask experts opinion on a decision I've made regarding a
function. I don't have my Scott Meyers at hand but I'm fairly sure he
mentions this issue.

consider virtual function;

void foo()
{

if (something)
{
do_x();
}
else
{
do_y();
}
}


Now for reasons we don't need to know in this post, do_x() is the
correct behaviour for the scenario I'm debugging. However as it stands
the code always goes to do_y();


I want to change the signature of the function to;

virtual void foo(bool myFlag = false);

I've also updated the other classes that override this virtual function
with the new default argument (no function hiding etc)..

Now in the code path that I'm interested in (to fix this bug), I call
foo(true). I make no changes to any other code, and so myFlag is always
false in that scenario.

The logic of the function now looks like;

void foo(bool myFlag = false)
{

if ((something) || myFlag)
{
do_x();
}
else
{
do_y();
}
}




Now this will work for me. However Scott Meyers (vaguely from memory) I
*think* says this is not a good idea. Would people out there agree? Is
this a recipe for disaster or a sound approach.

thanks much.

Graham

The problem with this scenario is that after you're done with the debugging
and fixed the function, your function foo() is now going to have a bool
parm. Any programmer coming along later (or even you later) are going to
wonder why the heck you have this bool parmater that isn't used.

If you're going to make sure to clean the function back up, it shouldn't be
a problem, except for the fact that 1 time out of 10 using this method
you'll forget. The problem then becomes you have code that makes no real
sense (why pass a bool parm that isn't used?) and someone is going to spend
time trying to figure out why before removing it, or they'll just leave it
alone for the next programmer to spend time trying to figure out.

If you insist on doing it this way, at least make the bool name indicative
of what it's there for.

void foo(bool ForDebugUseOnly = false)

 

[Dervish]

Why can't you create THREE functions:
1) void foo() - orginal one, calls fooImpl
2) void mySpecificFoo() - with new hacked logic, calls fooImpl
3) void fooImpl(bool myFlag) - legacy code + tricky new flag.

 

[roberts.noah]

Now for reasons we don't need to know in this post, do_x() is the
correct behaviour for the scenario I'm debugging. However as it stands
the code always goes to do_y();


I want to change the signature of the function to;

virtual void foo(bool myFlag = false);

Use the debugger. Is "something" a variable? If not make in one.
Then break after assignment to that variable or just before the if and
assign true to that variable. Then "continue" and there you go.

 

[Daniel T.]

Hi,


I'm working on fixing issues/bugs in a legacy codebase.

I'd like to ask experts opinion on a decision I've made regarding a
function. I don't have my Scott Meyers at hand but I'm fairly sure he
mentions this issue.

consider virtual function;

void foo()
{

if (something)
{
do_x();
}
else
{
do_y();
}
}


Now for reasons we don't need to know in this post, do_x() is the
correct behaviour for the scenario I'm debugging. However as it stands
the code always goes to do_y();

Change the code to call do_y() directly?
Fix the code so that 'something' is set correctly in all cases?

I want to change the signature of the function to;

virtual void foo(bool myFlag = false);

Do you really want to give every client the ability to bypass
'something' and call 'do_y' like that? If so, make do_y() public and let
them call it.

Now this will work for me. However Scott Meyers (vaguely from memory) I
*think* says this is not a good idea. Would people out there agree? Is
this a recipe for disaster or a sound approach.

I'm not sure what Scott Meyers would say, but I don't like it.

 

[Jerry Coffin]

(e-mail address removed) wrote:

[ ... ]

consider virtual function;

void foo()
{

if (something)
{
do_x();
}
else
{
do_y();
}
}

[ ... ]

I want to change the signature of the function to;

virtual void foo(bool myFlag = false);

I've also updated the other classes that override this virtual function
with the new default argument (no function hiding etc)..

Now in the code path that I'm interested in (to fix this bug), I call
foo(true). I make no changes to any other code, and so myFlag is always
false in that scenario.

There are a couple of concerns here.

One is about the basic structure of the program. You seem to want foo()
to do two fundamentally different things under different circumstances.
If you basically want the ability to call a portion of foo() directly,
you should probably restructure that part into a function of its own,
and then call it directly when needed, and have foo() call it when it
needs to. In this case, at least as you've shown the code here,
foo(true) is equivalent to calling do_x(), so it would almost certainly
be better to call do_x directly when/if that's what you really want. I
realize you may have simply condensed whatever was really into the if
statement and represented it as do_x, but if so, you should simply
create a do_x (with a suitable name, of course) and use it when that's
what you really want.

The second issue is with using a bool as a parameter. You might really
need (or at least want) to keep the code inside of foo (e.g. if foo
does other things before and/or after calling do_x or d_y, and it would
be difficult to separate out into a number of separate functions). In
this case, leaving the code as a single function may be perfectly
reasonable -- but you can almost certainly come up with something more
description than a bool to describe what it does in each case. If you
can represent the cases as a bool, then you can also represent them as
an enum with two possible values. The difference is that you can give
your enum a more meaningful name. Consider something like:

struct disk {
virtual void write(bool x = false) {
if (x || something)
write_to_disk();
else
write_bypassing_cache();
}

And then consider instead:

struct disk {
enum caching { cache, direct };

virtual void write(enum caching type = cache) {
if (type == cache || something) {
write_to_disk();
else
write_bypassing_cache();
}
};

Seeing a call to 'x.write(disk::direct)' gives me a fair idea of what
it's actually doing and what the parameter means. A call to
'x.write(true)' is far less informative.

 

[Howard]

Hi,


I'm working on fixing issues/bugs in a legacy codebase.

I'd like to ask experts opinion on a decision I've made regarding a
function. I don't have my Scott Meyers at hand but I'm fairly sure he
mentions this issue.

consider virtual function;

void foo()
{

if (something)
{
do_x();
}
else
{
do_y();
}
}


Now for reasons we don't need to know in this post, do_x() is the
correct behaviour for the scenario I'm debugging. However as it stands
the code always goes to do_y();


I want to change the signature of the function to;

virtual void foo(bool myFlag = false);

I've also updated the other classes that override this virtual function
with the new default argument (no function hiding etc)..

Now in the code path that I'm interested in (to fix this bug), I call
foo(true). I make no changes to any other code, and so myFlag is always
false in that scenario.

The logic of the function now looks like;

void foo(bool myFlag = false)
{

if ((something) || myFlag)
{
do_x();
}
else
{
do_y();
}
}




Now this will work for me. However Scott Meyers (vaguely from memory) I
*think* says this is not a good idea. Would people out there agree? Is
this a recipe for disaster or a sound approach.

Is this just for debugging purposes? If so, then the suggest from
"roberts.noah" is good.

If you want another way to run a specific test, without using the debugger,
you could use the preprocessor, like this:


#ifdef FOR_TESTING_FOO
if (true)
#else
if (something)
#endif
{
do_x();
}
else
{
do_y();
}
#endif

(I don't know what Mr. Meyers might have to say about your method, because
you haven't provided sufficient info to know what the topic he'd be
addressing is.)

-Howard

 

[Howard]

Is this just for debugging purposes? If so, then the suggest from
"roberts.noah" is good.

That should be "suggestion", not "suggest", naturally.

If you want another way to run a specific test, without using the
debugger, you could use the preprocessor, like this:


#ifdef FOR_TESTING_FOO
if (true)
#else
if (something)
#endif
{
do_x();
}
else
{
do_y();
}
#endif

Oh, and then simply define FOR_TESTING_FOO (in your project or command line
or whatever you use) when you want to force the do_x() behavior.

-Howard

 

[bob]

thanks for the responses. I think I should make more explicit the fact
that the logic contained in the code is *correct* when called from
elsewhere within the codebase. Under a certain scenario (i.e. for the
bug I fix) I need the code to call do_x(). i.e. When *I* call foo(bool
MyFlag =false ) I will do so using foo(true). All other *existing* code
will call foo() and therefore get the behaviour thats currently in
place. I need to make sure I call do_x()..... the "something" condition
is actually some very heavy quants mathematics dependent variable which
is greek to me (geddit... the greeks.

One is about the basic structure of the program. You seem to want foo()
to do two fundamentally different things under different circumstances.

You've grasped the issue here. I want it to behave EXACTLY as it is
EXCEPT when I call it from my bugfix code. Hence the approach taken.
When the function is called from the codepath I am fixing, "something"
is false. I need to have do_x() invoked when this "something" is false.
You see where I'm coming from.

Anyways, I think I have got the behaviour I need. The solution works as
desired. I've not been informed of any "side" effects of the addition
of the default argument so I am happy enough. The defaulted argument is
well documented and it works fine.

thanks for the responses.

Graham