What d'ya think?

D

Danny

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;
}
 
D

David White

Danny said:
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>
Click to expand...

Do you need said:
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
Click to expand...

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;
Click to expand...

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)
{
Click to expand...

How about a more descriptive name than 'calculate'?
cost = uni * (((((rat * hour) * day) * week) * month) * year);

return cost;
Click to expand...

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);
}
Click to expand...

DW
 
R

Raoul Gough

David White said:
Do you need <ostream>?
Click to expand...

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]
 
A

Allan Bruce

Danny said:
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);
Click to expand...

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
 
Q

!Q

Danny said:
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>
Click to expand...

already have iostream
using namespace std;
Click to expand...

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
Click to expand...

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);
Click to expand...

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
Click to expand...
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
Click to expand...

Why a new scope?
cout << "Enter the unit cost of electricity (pence): ";
Click to expand...

What if the I/O is buffered? Make sure you flush the stream!
cin >> unit;
Click to expand...

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);
Click to expand...

if you are only using cost here it should be a local variable
return cost;
Click to expand...

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..
}
Click to expand...

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
 
D

Danny

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.
 
D

Danny

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.
 
P

Patrick Frankenberger

!Q said:
What if the I/O is buffered? Make sure you flush the stream!
Click to expand...

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;
}
Click to expand...

Your "errorchecking" is not a bit better than his. std::cin >> input may
fail and that is what has to be checked.

HTH,
Patrick
 
D

Default User

John said:
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.
Click to expand...


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
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

What do you think about this script? 0
Please critique my code for fun learning project. 5
New coder looking for critique on fun project. 6
First time question 1
CIN Input #2 gets skipped, I don't understand why. 1
Chatbot 0
Getting a bad input error, not sure why. 2
Help with code plsss 0

Members online

No members online now.
Total: 2,565 (members: 0, guests: 2,565)
Robots: 145

Forum statistics

Threads
474,139
Messages
2,570,805
Members
47,351
Latest member
LolaD32479

Latest Threads

Top