445,849 Members | 2,164 Online Need help? Post your question and get tips & solutions from a community of 445,849 IT Pros & Developers. It's quick & easy.

# What d'ya think?

 P: n/a Hi Could somone please criticise the following program, it went together far to easily (bearing in mind i am just getting into c++) **note the tabs are not as they appear in my compliler** Dan /* This program will calculate the cost of running an electric appliance over several different time spans*/ #include #include using namespace std; double unit = 0; // unit cost of electricity double rate = 0; // the rating of the appliance double hours = 0; // number of hours per day double days = 0; // number of days per week double weeks = 0; // number of weeks a month double months = 0; // number of months a year double years = 0; // number of years double cost = 0; // cost of the appliance double calculate(double uni, double rat, double hour, double day, double week, double month, double year); int main () { cout << "The program has begun!" << "\n"; // indicates the program is alive { // obtains initial values cout << "Enter the unit cost of electricity (pence): "; cin >> unit; cout << "\n"; cout << "Enter the rating of the appliance (Kilo Watts): "; cin >> rate; cout << "\n"; cout << "Enter the number of hours: "; cin >> hours; cout << "\n"; cout << "Enter the number of days: "; cin >> days; cout << "\n"; cout << "Enter the number of weeks: "; cin >> weeks; cout << "\n"; cout << "Enter the number of months: "; cin >> months; cout << "\n"; cout << "Enter the number of years: "; cin >> years; cout << "\n"; cout << "The total cost over the specified time span is: " << calculate(unit, rate, hours, days, weeks, months, years) << " pence" << "\n"; } return 0; } // Function to do the specific calculation double calculate(double uni, double rat, double hour, double day, double week, double month, double year) { cost = uni * (((((rat * hour) * day) * week) * month) * year); return cost; } Jul 19 '05 #1
8 Replies

 P: n/a "Danny" wrote in message news:3F************@ntlworld.com... Hi Could somone please criticise the following program, it went together far to easily (bearing in mind i am just getting into c++) **note the tabs are not as they appear in my compliler** Dan /* This program will calculate the cost of running an electric appliance over several different time spans*/ #include #include Do you need ? using namespace std; double unit = 0; // unit cost of electricity double rate = 0; // the rating of the appliance double hours = 0; // number of hours per day double days = 0; // number of days per week double weeks = 0; // number of weeks a month double months = 0; // number of months a year double years = 0; // number of years double cost = 0; // cost of the appliance I wouldn't have all these global variables, although for a program like this I suppose it doesn't hurt. Still, you don't gain anything from it so I still wouldn't do it. I suggest making them locals in 'main'. Every place you access a global variable is like soldering a wire from there to the global variable. If your program grows to a reasonable size you end up with a mess of wires. This is supposed to be _soft_ware. You also get bugs because different functions are changing the same variable or because somewhere a function is called with the variables the wrong values, and you also get harder and harder-to-maintain code. double calculate(double uni, double rat, double hour, double day, double week, double month, double year); int main () { cout << "The program has begun!" << "\n"; // indicates the program is alive { // obtains initial values cout << "Enter the unit cost of electricity (pence): "; cin >> unit; Since you are inputting double after double, why not have a function such as: double enter_double(const char *prompt); which you call for each of your variables? Your main would be much shorter and look ten times neater. cout << "\n"; cout << "Enter the rating of the appliance (Kilo Watts): "; cin >> rate; cout << "\n"; cout << "Enter the number of hours: "; cin >> hours; cout << "\n"; cout << "Enter the number of days: "; cin >> days; cout << "\n"; cout << "Enter the number of weeks: "; cin >> weeks; cout << "\n"; cout << "Enter the number of months: "; cin >> months; cout << "\n"; cout << "Enter the number of years: "; cin >> years; cout << "\n"; cout << "The total cost over the specified time span is: " << calculate(unit, rate, hours, days, weeks, months, years) << " pence" << "\n"; } return 0; } // Function to do the specific calculation double calculate(double uni, double rat, double hour, double day, double week, double month, double year) { How about a more descriptive name than 'calculate'? cost = uni * (((((rat * hour) * day) * week) * month) * year); return cost; The global thing again: why use the global 'cost' when a local will do just as well? You are returning the calculated value, and main is receiving it. There's just no good reason not to have: double cost = uni * (((((rat * hour) * day) * week) * month) * year); return cost; or just return uni * (((((rat * hour) * day) * week) * month) * year); } DW Jul 19 '05 #2

 P: n/a "David White" writes: "Danny" wrote in message news:3F************@ntlworld.com... #include #include Do you need ? Strictly speaking, there is no guarantee that iostream will declare all of the standard operator<< overloads, so it is possible (in theory, anyway) for a compiler to require the explicit inclusion of ostream. [snip] -- Raoul Gough "Let there be one measure for wine throughout our kingdom, and one measure for ale, and one measure for corn" - Magna Carta Jul 19 '05 #3

 P: n/a "Danny" wrote in message news:3F************@ntlworld.com... Hi Could somone please criticise the following program, it went together far to easily (bearing in mind i am just getting into c++) **note the tabs are not as they appear in my compliler** Dan /* This program will calculate the cost of running an electric appliance over several different time spans*/ #include #include using namespace std; double unit = 0; // unit cost of electricity double rate = 0; // the rating of the appliance double hours = 0; // number of hours per day double days = 0; // number of days per week double weeks = 0; // number of weeks a month double months = 0; // number of months a year double years = 0; // number of years double cost = 0; // cost of the appliance double calculate(double uni, double rat, double hour, double day, double week, double month, double year); int main () { cout << "The program has begun!" << "\n"; // indicates the program is alive { // obtains initial values cout << "Enter the unit cost of electricity (pence): "; cin >> unit; cout << "\n"; cout << "Enter the rating of the appliance (Kilo Watts): "; cin >> rate; cout << "\n"; cout << "Enter the number of hours: "; cin >> hours; cout << "\n"; cout << "Enter the number of days: "; cin >> days; cout << "\n"; cout << "Enter the number of weeks: "; cin >> weeks; cout << "\n"; cout << "Enter the number of months: "; cin >> months; cout << "\n"; cout << "Enter the number of years: "; cin >> years; cout << "\n"; cout << "The total cost over the specified time span is: " << calculate(unit, rate, hours, days, weeks, months, years) << " pence" << "\n"; } return 0; } // Function to do the specific calculation double calculate(double uni, double rat, double hour, double day, double week, double month, double year) { cost = uni * (((((rat * hour) * day) * week) * month) * year); Beware of this - you have declared global variables and then passed some values to a function with the same name. In your case it works because you dont modify the values inside the function but if you had then your global variables would NOT be modified, which may not be the behaviour you want. Generally globals are not good to use, you dont need them here - you could have declared all the rat/hour/etc in the main and that would have sufficed. Perhaps a better solution would be to put these in a structure too since they are similar, e.g. typedef struct EmployeeStatsTag{ double rat; double hour; double day; double week; double month; double year; } EmployeeStats; This just makes life easier when you pass to a function, now you just need to pass one thing instead of 6 which you had. I think someone has already mentioned that you do a very similar operation to get input from the user, this could be made into a function with a for() loop which would be neater. HTH Allan Jul 19 '05 #4

 P: n/a Hi Using all the globals was just the best way that i could map it out in my mind, when ive finished a bit of refining i'll probably move them to locals in the places where they are needed. David White wrote: "Danny" wrote in message news:3F************@ntlworld.com...HiCould somone please criticise the following program, it went togetherfar to easily (bearing in mind i am just getting into c++) **note thetabs are not as they appear in my compliler**Dan /* This program will calculate the cost of running an electric applianceover several different time spans*/#include #include Do you need ?using namespace std;double unit = 0; // unit cost of electricitydouble rate = 0; // the rating of the appliancedouble hours = 0; // number of hours per daydouble days = 0; // number of days per weekdouble weeks = 0; // number of weeks a monthdouble months = 0; // number of months a yeardouble years = 0; // number of yearsdouble cost = 0; // cost of the appliance I wouldn't have all these global variables, although for a program like this I suppose it doesn't hurt. Still, you don't gain anything from it so I still wouldn't do it. I suggest making them locals in 'main'. Every place you access a global variable is like soldering a wire from there to the global variable. If your program grows to a reasonable size you end up with a mess of wires. This is supposed to be _soft_ware. You also get bugs because different functions are changing the same variable or because somewhere a function is called with the variables the wrong values, and you also get harder and harder-to-maintain code.double calculate(double uni, double rat, double hour, double day, doubleweek, double month, double year); int main (){cout << "The program has begun!" << "\n"; // indicates the program is alive{ // obtains initial valuescout << "Enter the unit cost of electricity (pence): ";cin >> unit; Since you are inputting double after double, why not have a function such as: double enter_double(const char *prompt); which you call for each of your variables? Your main would be much shorter and look ten times neater.cout << "\n";cout << "Enter the rating of the appliance (Kilo Watts): ";cin >> rate;cout << "\n";cout << "Enter the number of hours: ";cin >> hours;cout << "\n";cout << "Enter the number of days: ";cin >> days;cout << "\n";cout << "Enter the number of weeks: ";cin >> weeks;cout << "\n";cout << "Enter the number of months: ";cin >> months;cout << "\n";cout << "Enter the number of years: ";cin >> years;cout << "\n"; cout << "The total cost over the specified time span is: " <

 P: n/a Hi If you note the names of the variables passed into the funtion and those used within the funtion you'll notice that they are different, just one letter shorter as i needed something which meant the same(ish) as the first one for understanding. Allan Bruce wrote: "Danny" wrote in message news:3F************@ntlworld.com...HiCould somone please criticise the following program, it went togetherfar to easily (bearing in mind i am just getting into c++) **note thetabs are not as they appear in my compliler**Dan /* This program will calculate the cost of running an electric applianceover several different time spans*/#include #include using namespace std;double unit = 0; // unit cost of electricitydouble rate = 0; // the rating of the appliancedouble hours = 0; // number of hours per daydouble days = 0; // number of days per weekdouble weeks = 0; // number of weeks a monthdouble months = 0; // number of months a yeardouble years = 0; // number of yearsdouble cost = 0; // cost of the appliancedouble calculate(double uni, double rat, double hour, double day, doubleweek, double month, double year);int main (){cout << "The program has begun!" << "\n"; // indicates the program is alive{ // obtains initial valuescout << "Enter the unit cost of electricity (pence): ";cin >> unit;cout << "\n";cout << "Enter the rating of the appliance (Kilo Watts): ";cin >> rate;cout << "\n";cout << "Enter the number of hours: ";cin >> hours;cout << "\n";cout << "Enter the number of days: ";cin >> days;cout << "\n";cout << "Enter the number of weeks: ";cin >> weeks;cout << "\n";cout << "Enter the number of months: ";cin >> months;cout << "\n";cout << "Enter the number of years: ";cin >> years;cout << "\n"; cout << "The total cost over the specified time span is: " <

 P: n/a "!Q" wrote: "Danny" wrote in message news:3F************@ntlworld.com... cout << "Enter the unit cost of electricity (pence): "; What if the I/O is buffered? Make sure you flush the stream! cin >> unit; cout and cin are tie()ed by default which means every read from cin implies a flush of cout. #include int main() { double total = 0.0, input; std::cout << "main() started..." << std::endl; std::cout << "Enter the unit cost of electricity (pence): " << std::flush; std::cin >> input; // check input here total *= input; // repeat previous block for other inputs // calculate and output the total std::cout << "The total cost over the specified time span is: " << total << std::endl; return 0; } Your "errorchecking" is not a bit better than his. std::cin >> input may fail and that is what has to be checked. HTH, Patrick Jul 19 '05 #8

 P: n/a John Harrison wrote: Too many global variables. Global variables are the work of the devil. Every bug ever found (well nearly every one) can be traced back to a global variable. Wow! That's amazing. I haven't used global variables in programs for over 10 years, so I must have been writing nearly bug-free code all that time. I wish. Brian Rodenborn Jul 19 '05 #9

### This discussion thread is closed

Replies have been disabled for this discussion. 