Long story short: I’m trying to do a sentinel-controlled loop, which is highly unclear in my book. Here’s the code & results----PLEASE tell me what’s wrong and how to fix it!
Code: #include <iostream>
using std::cout;
using std::cin;
using std::endl;
int main()
{
int hrworked, rate, grosspay, hourCounter;
hourCounter=0;
while ( hrworked != -1) {
hourCounter = hourCounter + 1;
cout<<"Enter number of hours worked, -1 to end: ";
cin>>hrworked;
cout<<"Enter hourly rate: ";
cin>>rate;
if (hrworked < 40)
grosspay=hrworked*rate;
else
grosspay=(40*rate)+((hrworked-40)*(rate*1.5));
cout<<"This is the total pay "<<grosspay<<endl;
}
return 0;
}
Results:
Enter number of hours worked, -1 to end:
10
Enter hourly rate: 32
This is the total pay 320
Enter number of hours worked, -1 to end: -1
Enter hourly rate: 10
This is the total pay -10
Press any key to continue
Why is the darn -1 not working???
It looks to me like it’s because it only checks to see if hrworked is -1 at the beginning of the while loop. You want to put another check in there immediately after it reads in a new value for hrworked.
Another alternative is to use a do-while loop, putting the test at the bottom, but also within the loop use an if conditional to only process the rate and mathematical part of the loop in the event hr is not -1.
Personally, I prefer Flymaster’s solution asking for a first value of hrworked outside of (before) the loop and again inside but at the end of the loop, but as usual There Is More Than One Way To Do It (TIMTOWTDI).
Anyway, the reason the -1 entry didn’t work should be fairly evident. Everything happens in sequence. By the time the code that reads in the -1 is executed, the choice as to whether or not to enter the loop is already several statements previous. The loop does not stop instantly and magically the moment hrworked is -1. It enters/reenters the loop based on a test at the top of the loop when the top of the loop is reached, and at no other time.
Also, of course, the comment has already been made about uninitialized variables. Variables do not have some magic default value when they are declared, unless you give them an initial value. If you do not fill them, they could be anything.
Nah, don’t do that. Break is evil, and there’s (almost) never any reason to use it in a while loop. That’s why they invented do/while loops, or, more accurately, that’s why they invented the while loop. It builds that break right in to it.
double hrworked, rate, grosspay;
int hourCounter;
for(hourCounter=0; true; ++hourCounter)
{
cout<<"Enter hours worked (-1 to end): ";
cin>>hrworked;
if(hrworked<0.0) break;
cout<<"Enter hourly rate: ";
cin>>rate;
if(hrworked<=40.0)
grosspay=hrworked*rate;
else
grosspay=(40.0+(hrworked-40.0)*1.5)*rate;
cout<<"This is total pay: "<<grosspay<<endl;
}
Comments:[ol][]I use doubles for the hours, rate, pay because that seems more appropriate. I made the numerical constants doubles, likewise. I also factored the overtime pay calculation (reduces it from 5 to 4 ops).[]I put hourCounters as the counter of a for loop, because that seems how you’re wanting to use it. It’s defined outside the loop, so you can access it outside the loop.I use a true condition on the loop, and place the condition check inside the loop (as Khadaji does). This is a reasonable place for it since you read the variable in the middle of the loop. As has been suggested, you could set the variable before the loop. I prefer to keep assignments and checks closer together for simpler code. As Flymaster suggested, you could read the value before entering the loop. But this then requires two separate read statements (this is a problem if you decide to change the code, then you must change it in two places). Using a break here is an easy way to make the logic very simple to understand.[/ol]
I disagree. Putting the break near where you want makes it much more readable. You want to break near the place you read the condition. And, when you are stepping through code with your debugger (something you shoud ALWAYS do, no matter how good you think you are) you can see exactly what caused the loop to break.
At the risk of pointing out what we shouldn’t be posting full source code solutions to someone’s homework, and then breaking that myself, I’ll post my algorithm, which doesn’t involve breaks or additional booleans or anything else. I think this outlines the basic solution posted by Flymaster, and is possibly the most elegant solution:
read a value into x
while (x is not the sentinel)
{
…
…
do something with x
…
…
read a value into x
}
One of the cleaner solutions. If I were to pick any other solution here… well, I probably would recommend something halfway between Khadaji and Pleonast:
set x to a non-sentinel value initially
while (x is not the sentinel)
{
read a new value into x
if x is not the sentinel
{
…
do something with x
…
}
}
or better:
do
{
read a new value into x
if x is not the sentinel
{
…
do something with x
…
}
} while (x is not the sentinel)
These are both relatively clean, don’t involve any additional awkward booleans, and say pretty much what is meant by the intent of the loop (translates from an English description of the intent of the problem quite well).
I can’t say I believe break is evil (judiciously used) or that falling out of a loop naturally by way of extra conditions is a problem (especially if its fairly clearly written, and you’re several layers deep in loops or switches). For whatever my opinion’s worth.
Except for possibly the treacherous overpowered goto, I tend to use the tools in a maintainable easy to read way… but then I have to write and maintain large amounts of code for a living, so I’m sensitive to overcleverness. (Check the sig)
As an ex-C++ instructor, let me say that the “officially preferred” method is what Flymaster suggests. It’s verbose but considered good practice for newbie programmers. The books will also suggest the “if”-protected loop body method a la MR2001. Note that “while(1)” is totally baffling to novices and generally avoided in beginner courses.
Both methods would be avoided in real life by real programmers who would use the break. Note that the verbose method means that if you change something about the sentinel you have to make the same change in two places.
Booklover, your compiler should have caught the unintialized value and warned you about it. (I know g++ would.) Check the flags to your compiler and and turn on those that give you warnings about such stuff.
Funny, I’m a ‘real programmer’ writing ‘real programs’ and I still prefer what FlyMaster suggests (unless the cost of twice setting up the value being tested is truly onerous and then … I’d probably use the break method).
For me, it’s not a question of how onerous setting the value is. It’s more that I don’t like repeating code. Each repetition is a new place for a bug to enter whenever that repeated code needs to changed.
If I truly wanted to use Flymaster’s logic (and it does have some appeal), I’d create a small function that would read in the value. I could then call that one function from as many places as necessary. And, if the read process needed to change, fixing the function would have effect in both places.
One homework problem won’t make a difference in the long run. And this thread is an excellent example of how real programmers can approach a problem from different angles.
< philosophy >
‘Just one’ homework problem is just as bad as many – its a bad habit to get into. If everyone answers explicitly ‘just one’ homework problem, pretty much all the C++ questions that come to this board will be explicitly answered. There are more answerers than questioners by a wide margin.
I’ve learned over the years explicit examples are more valuable to some people than others when learning this stuff. Fair enough. Myself, I try to restrict myself to pseudocode where I can gloss over those bits of the code that don’t directly deal with the question at hand. I find pseudocode even helps some people in the mental process of translating their intent to code. Most of time time I try to nudge and hint before I even get to pseudocode. But that’s just me.
< /philosophy >
Having vented a little, I agree – it is fascinating seeing the different angles of attack, and evaluating the benefits and detriments. Its rare in my workplace to have a fine debate on the academic and practical art we practice. I’m sure the original poster has a lot to chew on.
I agree with this, actually, so long as there’s enough code in which a bug may exist. A single output statement followed by a single input statement, duplicated as in the example, doesn’t leave a lot of room for a bug. But I suppose it doesn’t take much before I can no longer reasonably make that claim.
I agree with this too. I’m not sure I’d go this far if it was as simple a situation as I just described. A one-line call to a function, or a two-line prompted input, isn’t much different in my books in terms of complexity. But once you start drifting outside of the extremely simple, I’m with you on non-duplication. I hate fixing things twice.
But, Pleonast, that “small function” has already been written for you. The extraction operator ( >> ), is nothing more than a function. You can overload it all you want, yadda yadda. There’s no need to write a function for this. If, for some reason, you wanted to change the way that you read in an integer to reading in an object of some kind, you just overload >> and it’ll be done for you. There’s no need for a separate wrapper.
There would be if it were a more complex bootstrapping procedure for the loop, say something that needs to read in several variables, do lots of prompting, some calculations, etc.
In this case (for the sake of the OP), you’re right. In a more complicated case, it might benefit wrapping up the mechanism being used, if you wanted to use the logic in question.
And actually, this example is sufficiently complex. We’re not only reading in a variable, we’re also outputing a prompt. What if you want to change the prompt? You’ll have to change the string in two places. Although mismatched prompts is not an especially nasty bug, it demonstrates the problem of have the “same” code in more than one place.
(Check the sig)
