Expert: Ralph McArdell - 3/7/2004

Question
this is all messed up. i dont know what to do. please help

void exercise23()
{
int compOne;
int SecNum;
cout<<"Here are some hailstone numbers: ";
cout<<endl;
do
{
for(int i=1;i<=200;i++)
{
compOne = i;
if (compOne%2 != 0)
{
SecNum = ((compOne*3)+1);
cout<<SecNum<<", ";
compOne = SecNum;
}
else if (compOne%2 == 0)
{
SecNum = compOne/2;
cout<<SecNum<<", ";
compOne = SecNum;
}
}
}while(compOne != 1);
cout<<endl;
}

int main(int argc, char* argv[])
{

  exercise23();

  return 0;
}

Answer
Neither do I. I do not know what exercise 23 is about nor do I recognise the term "hailstone numbers" - it has some specialist meaning to you I presume.

So I can only offer general advise as I do not know what you are trying to achieve.

The core of your program appears to be doing the following:

While compOne not 1:
       Iterate from 1 to 200 in increments of 1:
               Set compOne to the iteration value
               If compOne odd:
                       Do one calculation to produce SecNum
               Else if even:
                       Do other calculation to produce SecNum
               Output SecNum
               Set compOne to SecNum
       End iteration
End while

Here are some points:

If an integer is not odd it is even so the if after the else is redundant.

The output of SecNum and the assignment of compOne from SecNum is repeated in both the if and the else portions so is common to all code paths so can be moved _out_ of these clauses and placed only once after the if ... else actions.

compOne is probably redundant as well, at least in the code you have at the moment as you can use the for loop variable i in its place to produce SecNum and you can test SecNum instead of compOne in the while statement. Is this really what you intended?

Your calculations produce 3i + 1 if odd and i/2 if even. So your sequence starts 4, 1 and more importantly the last values are 598, 100. This final value is very important as compOne is set to this value, then the for loop terminates and compOne is tested to see if it is _not_ 1, compOne is 100 so this test is _true_ so round we go again, and again and again, ...

I suggest you examine the reason for the do ... while loop at all and definitely fix the loop continuation condition. I suspect it is related to my query about your usage of compOne - that you are _not_ setting compOne correctly in the first place - however as I do not know what you are trying to do anyway I could well be wrong.

On general C++ things:

You appear not to include any required headers - such as <iostream> for example.

The names cout and endl live in the std namespace so should be specified as such: std::cout and std::endl - however you may be using an old C++ implementation before this became the standard (in 1998).

Do you really need to specify endl at all? std::endl write a newline and flushes the stream - would just writing a newline be sufficient?

       std::cout << "\n";

or:

       std::cout << "Here are some hailstone numbers:\n";


The arguments to main are not used so you are permitted to specify main like so:

       int main()

Also main need not return a value - if you do not do so then it is as if return 0 was executed, however some compilers get this wrong. Note that this behaviour applies _only_ to main.

You will find that in C++ we prefer pre-increment (++i) and decrement (--i) to post increment (i++) and decrement (i--) in situations where it does not matter which is used. This is because in C++ we can overload these operations for our own types in which case we may well be calling a function for a user defined type rather than the built in operations for the built in types, and it turns out that the post increment and decrement forms are generally more expensive to implement. Admittedly when incrementing an int as in this case there is little to choose either way on the performance side but it does matter in other situations - so we like to keep in the habit anyway!

On general development things:

You can see what is going on in your program using several techniques - dry running it keeping the state in your head (as I started doing here) or noted down on paper being the most obvious. You have to run the code as _written_ not as you thought you wrote it!

Or you could use a debugger - which I did having built your code I then stuck a breakpoint on the while statement and ran it to make sure I had got the behaviour correct (I had). The debugger stops execution when it hits the breakpoint and allows me to do many useful things such as execute the code one line at a time or examine the value of variables - compOne in this case. Indeed it was 100 every time around the loop and indeed the program would run until stopped - another thing you can do from a debugger. I suggest to find out what debugging facilities are available to you and what you need to do to create a debug build - to enable all this functionality compilers add a whole load of additional "debug" information to the code they produce - so producing such code is optional - and some compilers can generate debug information in various formats for different debuggers.

I do note know if this next point applies, but you should always indent you code to reflect the structure of the code. Your code for example23() arrived all against the left hand margin. Whether this is how you wrote the code or is an effect of posting via AllExperts I do not know.

You may not wish to place the introductory text in the same function as the work is done. I suggest you either move the "Here are some hailstone numbers:" to main then call example23 or split example23 into two functions, one that just output the introduction the calls the other, which does the work. In general this make you functions much more useful as you are not tied to only using them where you want that text output in that way - maybe later you wish to generate only the values but without the introductory text...