By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
445,849 Members | 2,164 Online
Bytes IT Community
+ Ask a Question
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 <iostream>
#include <ostream>

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
Share this Question
Share on Google+
8 Replies


P: n/a
"Danny" <da***********@ntlworld.com> 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 <iostream>
#include <ostream>
Do you need <ostream>?

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" <no.email@provided> writes:
"Danny" <da***********@ntlworld.com> wrote in message
news:3F************@ntlworld.com...
#include <iostream>
#include <ostream>


Do you need <ostream>?


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" <da***********@ntlworld.com> 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 <iostream>
#include <ostream>

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
!Q

"Danny" <da***********@ntlworld.com> 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 <iostream>
#include <ostream>
already have iostream
using namespace std;
hrm, they're in a namespace for a reason... I prefer to explicitly state the
namespace to avoid potential confusion (when programs become more complex
this avoids confusion)
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
Try not to polute the global namespace, especially with variables. These
variables will be able to be accessed from _anywhere_ in the code (using
extern) so tracking down bugs would be increasingly difficult. Confine the
scope of variables (declare and use them at the latest possible/practical).
Also I would store these values in a structure (if you were writing a real
world program you would not be doing something so simple so a better level
of abstraction would be needed (type systems)).

double calculate(double uni, double rat, double hour, double day, double
week, double month, double year);
This function is now exported to other modules, does it need to be? You can
use the "static" keyword to indicate that the function should have internal
linkage only. Alternatively, the purists will prefer this, you can use an
unnamed namespace ;-).
int main ()
{

cout << "The program has begun!" << "\n"; // indicates the program is alive

Your comments don't seem to add value, comments are normally only needed to
explain tricky, non boiler-plate code (little alogirthmic tricks you may
use).
{ // obtains initial values
Why a new scope?
cout << "Enter the unit cost of electricity (pence): ";
What if the I/O is buffered? Make sure you flush the stream!
cin >> unit;
All these are missing error checking routines (what if the user enters an
incorrect value? you need to validate the input).
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);
if you are only using cost here it should be a local variable
return cost;
oh, you are just returning the above value no local variable necessary just
return it !!

return uni * (((((rat * hour) * day) * week) * month) * year);

the floating point multiplication operator is associative therefore the use
of brackets is superfluous, ie:

return uni * rat * hour * day * week * month * year;

hrm, do you think this function is needed at all? rather simple..
}


The program you have written is a good first attempt ;-). At this stage a
lot of the stuff I have mentioned is probably not important to you, I have
simply mentioned it to give you a feel for what "real" software is
structured like. I would say the first thing you should do is to insert some
error handling routines (make sure the input is valid). Also make your
variables local!! This will mean only a limited amount of code will have
access to them (so when you are looking for problems in your code there will
be less lines of code to look at). In your program almost all of the
variables are not needed. What you could do is have two variables, one that
keeps the intermediate results of the final calculation. The other variable
would be for input. Maybe something like this:
#include <iostream>

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;
}
That was a quick hack because I've got go watch t.v. :-). As an exercise
spot the errors. Have fun coding!

!Q
Jul 19 '05 #5

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" <da***********@ntlworld.com> 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 <iostream>
#include <ostream>

Do you need <ostream>?

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 #6

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" <da***********@ntlworld.com> 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 <iostream>
#include <ostream>

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 #7

P: n/a

"!Q" wrote:

"Danny" <da***********@ntlworld.com> 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 <iostream>

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.