473,396 Members | 1,789 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,396 software developers and data experts.

What d'ya think?

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
8 1929
"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
"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

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

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


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 thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

220
by: Brandon J. Van Every | last post by:
What's better about Ruby than Python? I'm sure there's something. What is it? This is not a troll. I'm language shopping and I want people's answers. I don't know beans about Ruby or have...
125
by: Sarah Tanembaum | last post by:
Beside its an opensource and supported by community, what's the fundamental differences between PostgreSQL and those high-price commercial database (and some are bloated such as Oracle) from...
145
by: Mark Johnson | last post by:
Oddly enough, I found it difficult, using Google, to find a list of best-of sites based on the quality of their css packages. So I'd ask. Does anyone know of particularly good sites which are in...
20
by: Wayne Sutton | last post by:
OK, I'm a newbie... I'm trying to learn Python & have had fun with it so far. But I'm having trouble following the many code examples with the object "self." Can someone explain this usage in...
23
by: JDeats | last post by:
Just spent some time browsing around here: http://msdn.microsoft.com/Longhorn/ I can see the benefits from WinFS (as long as we tag all in-coming data this should be nice, tagging everything...
10
by: sherifffruitfly | last post by:
Hi, I'm just learning cpp, and the exercise I'm working on is basically as follows: 1) Create a struct type with 4 members (char, char, char, int). 2) Create an array of, say 3 instances of...
5
by: Nirjhar Oberoi | last post by:
Hi, Question 1: I want your help on choosing what OS should one use to learn C Programming? I was thinking of installing Linux because a large no of tools are preconfigured in it.. I just...
4
by: Boki | last post by:
Hi All, I have a timer, if my data queue Q has data, the timer should start work, if there is no data in Q, the timer should stop. However, there is an event can fire timer to start. Should I...
12
by: Darrel | last post by:
I'm still having a hell of a time figuring out this whole SQL Express set up. I finally discovered why I couldn't run the aspnet_regsql...my local sql server wasn't running. I turned that on,...
0
by: Charles Arthur | last post by:
How do i turn on java script on a villaon, callus and itel keypad mobile phone
1
by: nemocccc | last post by:
hello, everyone, I want to develop a software for my android phone for daily needs, any suggestions?
1
by: Sonnysonu | last post by:
This is the data of csv file 1 2 3 1 2 3 1 2 3 1 2 3 2 3 2 3 3 the lengths should be different i have to store the data by column-wise with in the specific length. suppose the i have to...
0
by: Hystou | last post by:
There are some requirements for setting up RAID: 1. The motherboard and BIOS support RAID configuration. 2. The motherboard has 2 or more available SATA protocol SSD/HDD slots (including MSATA, M.2...
0
marktang
by: marktang | last post by:
ONU (Optical Network Unit) is one of the key components for providing high-speed Internet services. Its primary function is to act as an endpoint device located at the user's premises. However,...
0
by: Hystou | last post by:
Most computers default to English, but sometimes we require a different language, especially when relocating. Forgot to request a specific language before your computer shipped? No problem! You can...
0
jinu1996
by: jinu1996 | last post by:
In today's digital age, having a compelling online presence is paramount for businesses aiming to thrive in a competitive landscape. At the heart of this digital strategy lies an intricately woven...
0
tracyyun
by: tracyyun | last post by:
Dear forum friends, With the development of smart home technology, a variety of wireless communication protocols have appeared on the market, such as Zigbee, Z-Wave, Wi-Fi, Bluetooth, etc. Each...
0
agi2029
by: agi2029 | last post by:
Let's talk about the concept of autonomous AI software engineers and no-code agents. These AIs are designed to manage the entire lifecycle of a software development project—planning, coding, testing,...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.