«

CODE COMPLETE, page 356

if(StatusOK)
{
if(DataAvail)
{
ImportantVar = x;
goto MID_LOOP;
}
}
else
{
ImportantVar = GetVal( );
MID_LOOP;

/* lots of code */
. . .
}

* * * * * * end fragment * * * * *

The idea here is to rewrite the code without the goto LABLE.

First we identify the Work.
Initialize or re-initialize the variable ImportantVar,
conditionally,
in the if or the else clause.
And,
execute /* lots of code */ unconditionally, no matter what the
conditions are.

Reading the code is not the problem, or even writing code.
The real problem is in divining the intent of the coder.

Let us say the intent (IntentA) is to do the Work under any
conditions.
Then this fragment has a problem.
In the event that SatusOK is true, but DataAvail is false, then the
outer
if will ( since its own test is ture ) execute its code block, the
inner if,
but will fall thru, doing no Work.
The else is the companion clause of the outer if, and since the if
tried
to execute, the else will be skipped.
No Work will be done at all.

Either this is a defect, or we have some other intent (IntentB):
Do the Work under all conditions, except *this one*.

Here is my rewirte for IntentA, a bit simpler than what McConnell
offered.
**********************************
If( StatusOK && DataAvail )
{
ImportantVar = x;
}
else
{
ImportantVar = GetVal( );
}

/* lots of code */ // What is the sense of putting
unconditionally

wrote:
> CODE COMPLETE, page 356
> if(StatusOK)
> {
> if(DataAvail)
> {
> ImportantVar = x;
> goto MID_LOOP;
> }
> }
> else
> {
> ImportantVar = GetVal( );
> MID_LOOP;
>
> /* lots of code */
> . . .
> }
>
> * * * * * * end fragment * * * * *
>
[much snippage below]

> The idea here is to rewrite the code without the goto LABLE.
>
> Reading the code is not the problem, or even writing code.
> The real problem is in divining the intent of the coder.
>
> Let us say the intent (IntentA) is to do the Work under any
> conditions.
>
> Either this is a defect, or we have some other intent (IntentB):
> Do the Work under all conditions, except *this one*.
["This one" being (StatusOK && !DataAvail)]

Good analysis, but I disagree with your conclusions. Changing behaviour
is a dangerous game in program maintenance.

You identify two possible intents (which you call IntentA and IntentB)
in what the code /should/ do. You correctly note that IntentB is what
the code actually does, though IntentA is possibly what the code should do.

Now there are four possibilities:

1. You change behaviour to IntentA and IntentA is correct:
Congratulations, you have removed the "goto" and corrected a bug at
the same time!

2. You change the behaviour to IntentA but IntentB is correct:
Oh dear! You have removed the goto, but introduced a bug. The
program is neater but is now incorrect. (see footnote [1]) This is a
regression, which is a Bad Thing.

3. You keep the behaviour as IntentB but IntentA is correct:
Well done! You have improved the code by removing a goto, and you
have neither introduced nor fixed any bugs. Possibly more work needs to
be done.

4. You keep the behaviour as IntentB and IntentB is correct:
Congratulations, you have removed the "goto"! The code is now neat
and correct.

Now, these possibilities are not equiprobable, but it can be seen that
by maintaining the current behaviour (IntentB) you cannot introduce any
new bugs into the program. Possibilities 1. and 4. are both ideal - the
program ends up neater and bug-free. The interesting ones are
possibilities 2. and 3.: the program is neater but incorrect. However,
in possibility 2., you started with a perfect program and you broke it
by introducing a bug; in possibility 3., you started with a buggy
program and made the code neater. 3. is an improvement, while 2. is a
regression.

By keeping the behaviour the same, only 3. or 4. are possible, and both
are Good Things. By changing the behaviour, 1. or 2. are possible; 1. is
Good, but 2. is Bad.

What this boils down to is you need to be absolutely sure that IntentA
is the correct behaviour before you attempt to modify the code's
behaviour. Otherwise you run the risk of introducing bugs into a program
which was working fine before. How can you be sure what the correct
behaviour is? You look at the formal specification, you examine the code
in context, you look at what the caller expects this code to do. Since
this is an exercise, I doubt that a formal spec exists, or even a
caller. So I'd play it safe and keep the behaviour the same.

DISCLAIMER: I haven't read "Code Complete"; I don't know the wider
context of the exercise. This is based entirely on what you wrote in
your article.

Philip

[1] "Complex problems often have simple, easy to understand, wrong answers."

writes:

> CODE COMPLETE, page 356
>
> if(StatusOK)
> {
> if(DataAvail)
> {
> ImportantVar = x;
> goto MID_LOOP;
> }
> }
> else
> {
> ImportantVar = GetVal( );
> MID_LOOP;

Presumably this is intended to be MID_LOOP:

> /* lots of code */
> . . .
> }
>

The sort of "obvious" re-write is just to separate the setting of teh
variable from the execution of the labeled code block. To my mind it
makes thing much clearer. Personally, I'd invert the first test
because it makes the setting simpler:

if (!StatusOK)
ImportantVar = GetVal();
else if (DataAvail)
ImportantVar = x;

if (!StatusOK || DataAvail) {
/* lots of code */
}

Of course, if the tests are complex so that re-evaluating them is not
acceptable then I'd be thinking about adding a new function.

--
Ben.