Feedback on my style

[dydx13]

Hello there,

Here is a program that I wrote in Visual Studio 2005:

--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

//Programmer: Nathan D. Brown
//Date: 3-1-07
//This is exercise 2.19 from the Deitel book.

#include<iostream>

using std::cin;
using std::cout;
using std::endl;

int main()
{
double hworked;
double hrate;
double salary;

cout<<"Please enter hours worked ( -1 to exit):";
cin>>hworked;

if(hworked == -1){
cout<<"No records were processed.";
}
else{
while(hworked != -1){
cout<<"Please enter hourly rate:";
cin>>hrate;
if(hworked > 40){
salary = (hworked - 40) * (hrate * 1.5) + (hworked - (hworked - 40)) * hrate;
}
else
salary = hworked * hrate;
cout<<"Employee salary is "<<salary<<endl;
cout<<"Please enter hours worked ( -1 to exit):";
cin>>hworked;
}
}

return 0;


}


-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


If you could please give me some feedback on my programming style, I would appreciate it.


Thank you,

Nathan

 

[Alf P. Steinbach]

* (e-mail address removed):

Hello there,

Here is a program that I wrote in Visual Studio 2005:

--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

//Programmer: Nathan D. Brown
//Date: 3-1-07
//This is exercise 2.19 from the Deitel book.

#include<iostream>

using std::cin;
using std::cout;
using std::endl;

int main()
{
double hworked;
double hrate;
double salary;

cout<<"Please enter hours worked ( -1 to exit):";
cin>>hworked;

if(hworked == -1){
cout<<"No records were processed.";
}
else{
while(hworked != -1){
cout<<"Please enter hourly rate:";
cin>>hrate;
if(hworked > 40){
salary = (hworked - 40) * (hrate * 1.5) + (hworked - (hworked - 40)) * hrate;
}
else
salary = hworked * hrate;
cout<<"Employee salary is "<<salary<<endl;
cout<<"Please enter hours worked ( -1 to exit):";
cin>>hworked;
}
}

return 0;


}

Please replaces tabs with spaces before posting on Usenet.

I'll only comment on what you can improve.

First, you have a magic number, namely 40, in there. It should be a
named constant.

Second, the expression (hworked - (hworked - 40)) is identical to 40,
except for the academic possibility of overflow.

Third, you have duplicated code, the "Please enter hours worked" part.
That suggests using a different loop, one with exit in the middle. Like

for( ;; )
{
cout << "Please ..."; cin >> hworked;
if( hworked == -1 )
{
break;
}
// ...
}

Hth.,

- Alf

 

[dydx13]

"Please replaces tabs with spaces before posting on Usenet."

What do you mean by this????????? I'm confused.

Nathan

 

[Pete Becker]

if(hworked > 40){
salary = (hworked - 40) * (hrate * 1.5) + (hworked - (hworked - 40)) * hrate;
}
else
salary = hworked * hrate;

In addition to having all those hard-coded values, this code is more
complicated than it needs to be. I'd do it like this:

if (hworked > 40)
hworked += (hworked - 40) * 0.5;
salary = hworked * hrate;

Now salary is only calculated in one place, making it much easier to see
what's going on.

--

-- Pete
Roundhouse Consulting, Ltd. (www.versatilecoding.com)
Author of "The Standard C++ Library Extensions: a Tutorial and
Reference." (www.petebecker.com/tr1book)

 

[mlimber]

"Please replaces tabs with spaces before posting on Usenet."

What do you mean by this????????? I'm confused.

He means your indenting makes the code hard to read in this group.
Prefer 2-4 spaces, not 8.

Cheers! --M

 

[Jerry Coffin]

[ ... ]

cout<<"Please enter hours worked ( -1 to exit):";
cin>>hworked;

You may not have reached the point in the book where they've taught you
how to do so, but if you know about functions, I'd write a small one to
do this part, something like:

double get_double(std::string const &prompt) {
std::cout << "Please enter " << prompt;
double ret;
std::cin >> ret;
return ret;
}

The rest could benefit from using descriptive names for more of what
you're doing:

double full_time = 40.0;
double overtime_multiplier = 1.5;

double overtime_hours = 0.0;

if (hworked > full_time) {
hworked = full_time;
overtime_hours = hworked - standard_hours;
}

double standard_pay = rate * standard_hours;
double overtime_pay = rate * overtime_multiplier * overtime_hours;

double salary = standard_pay + overtime_pay;

As far as the structure of the program goes, I don't like the fact that
you ask the user to the hours worked in two different places. Above,
I've used a function to isolate most of that into one place, but that's
still not really sufficient (IMO). I'd prefer a structure more like:

while (-1.0!=(hworked=get_double("hours worked (-1 to exit): ") {
double rate = get_double("hourly rate: ");
std::cout << "Salary is: " << compute_salary(hours, rate);
}

where compute_salary contained code similar to the previous computation.

 

[Jerry Coffin]

[ ... ]

The rest could benefit from using descriptive names for more of what
you're doing:

double full_time = 40.0;
double overtime_multiplier = 1.5;

double overtime_hours = 0.0;

if (hworked > full_time) {
hworked = full_time;
overtime_hours = hworked - standard_hours;

Oops -- that should be:

overtime_hours = hworked - full_time;

Sorry 'bout that.