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