-
Couple of questions
Hi I have to learn c++ for uni and so here I am. I had been programming in c before so its not as hard but I'm trying to make sure I do things the right way for c++ rather than for c. This is my first attempt and I wanted to check that I haven't done anything in a c way or badly.
By the way I had a seperate function to add pizza's because I'm planning on having a linked list of them. Is having a function seperate to the class the right way of doing this? Also should I put the next and prev pointers in the class? I'm asking because I don't want to start bad habits.
Thanks, David
Code:
#include <iostream>
#include <stdlib.h>
using namespace std;
const double PIE = 3.14;
// Define the cPizza class
class cPizza
{
/* add_pizza(int diameter, double cost);*/
double radius, area;
double cost;
public:
double value(void);
cPizza(int diameter, float temp_cost);
};
// The contructor for cPizza
cPizza::cPizza(int diameter, float temp_cost)
{
radius = diameter / 2;
cost = temp_cost;
}
// Calculates the value for the pizza
double cPizza::value()
{
float cost_per_inch;
area = (radius * radius) * PIE;
cost_per_inch = area / cost;
return cost_per_inch;
}
cPizza* add_pizza(int diameter, double cost);
int main(int argc, char *argv[])
{
int diameter;
float radius, area, value;
double cost;
cPizza *pizza;
cout << "Pizza calculator\n\n";
cout << "Size of pizza (Diameter in inches.): ";
cin >> diameter;
cout << "Cost of pizza (Pounds): ";
cin >> cost;
/*cPizza Pizza(diameter, cost);*/
pizza = add_pizza(diameter, cost);
cout << "The pizza costs £" << pizza->value() <<" per square inch\n";
delete(pizza);
system("PAUSE");
return 0;
}
cPizza* add_pizza(int diameter, double cost)
{
cPizza *new_pizza;
new_pizza = new cPizza(diameter, cost);
return new_pizza;
}
-
Well, if you have a class Pizza that encapsulated the properties of a pizza, it wouldn't make sense to put an "add" function on it to add a pizza to a pizza. Maybe an "addToppings" function, but not an "addPizza" function. Maybe you could have a PizzaShoppingCart class that would contain a linked list of pizzas or something like that, on that class you would add an addPizza function.
By the way, I love your:
Code:
const double PIE = 3.14;
:D
-
c++ prefers either vector or list instead of C's linked list techniques. And you don't need next and previous pointers. Simple example:
Code:
#include <vector>
std::vector<pizza> array;
-
If you want to do things the C++ way, then don't make your own linked list, use the one from the standard library: std::list (std::vector or std::deque might also be appropriate depending on what you plan to do). A separate class that maintains that list is a good idea instead of making it global.
Your cPizza class is pretty easy to copy (its just a few doubles), so instead of using new to allocate each pizza, I'd just store them in the list (or vector or deque) directly. That way you don't need to remember to delete them later.
I would also just use double everywhere instead of float, I don't see a reason to mix them up.
-
Code:
#include <stdlib.h>
The above version of that particular header file is deprecated, you should prefer this version of the header file:
-
However, since this is for a University course, they may actually WANT you to create your own list someday, if no other reason than it's a common way to gain confidence about using pointers, so when you go to do things that aren't in STL, like building your own tree class, or some such project, you've actually had the experience, even if you don't remember the specifics.
So, one way to create your own list class in C++ would be to have three separate classes. One class would be to hold just the information you want to store, in this case a Pizza class. The second class would be to hold the information plus pointers to the next, previous or whatever other pointers you want to use. This is frequently called a node or a link class. The third class would be the actual list class which would hold a group of nodes, with the Pizza information enclosed in the nodes rather than raw Pizzas, as you would in the STL list class. The methods to add/delete/swap whatever Pizzas to/from/within would be within the list class, but you would have to write them, as opposed to using the prewritten forms from the STL list class, etc.
It's good practice if you want (or are required) to go that route. Have fun.
-
Code:
cPizza* add_pizza(int diameter, double cost)
{
cPizza *new_pizza;
new_pizza = new cPizza(diameter, cost);
return new_pizza;
}
Could better be written as....
Code:
cPizza* add_pizza(int diameter, double cost)
{
return new cPizza(diameter, cost);
}
area should be initialised in the constructor. Always make sure you initialise all members.
get out of the c habit of declaring at top of block. in c++ the preferred method is to give each item its minimal possible scope. i.e.
Code:
double cPizza::value()
{
float cost_per_inch;
area = (radius * radius) * PIE;
cost_per_inch = area / cost;
return cost_per_inch;
}
should be
Code:
double cPizza::value()
{
area = (radius * radius) * PIE; // this should be in constructor
double cost_per_inch = area / cost;
return cost_per_inch;
}
you could even get rid of the named variable too and just return area/cost. minimal scopes and riddance of unnecessary variables makes bug hunting easier.
You could halve that code, have same result and you will find its much more readable too.
-
Thanks for the replys. Here is the new version of my code. I tried to change things as you said to.
It doesn't compile because of the "pizza_list.push_front(pizza);". I think it says that pizza shouldn't be a pointer, but I don't see how else to do it. If I made pizza not a pointer it would work but if I wanted to say add 10 different pizza's I would have to use pointers surely. Any ideas?
Code:
#include <iostream>
#include <cstdlib>
#include <list>
using namespace std;
const double PIE = 3.14;
// Define the cPizza class
class cPizza
{
double radius, area;
double cost;
public:
double value(void);
cPizza(int diameter, double temp_cost);
};
// The contructor for cPizza
cPizza::cPizza(int diameter, double temp_cost)
{
radius = diameter / 2;
area = (radius * radius) * PIE;
cost = temp_cost;
}
// Calculates the value for the pizza
double cPizza::value()
{
return area / cost;
}
int main(int argc, char *argv[])
{
int diameter;
double cost;
cPizza *pizza, *current_pizza;
list <cPizza> pizza_list;
cout << "Pizza calculator\n\n";
cout << "Size of pizza (Diameter in inches.): ";
cin >> diameter;
cout << "Cost of pizza (Pounds): ";
cin >> cost;
// Add a pizza
pizza = new cPizza(diameter, cost);
pizza_list.push_front(pizza);
cout << "Size\t" << "Cost\t" << "Value\n";
// Display all the pizza's
for(current_pizza = pizza.begin(); current_pizza != pizza_list.end(); ++current_pizza)
{
cout << current_pizza->diameter << "\t" << current_pizza->cost << "\t" <<current_pizza->value() << "\n";
}
// Delete them all
for(current_pizza = pizza.begin(); current_pizza != pizza_list.end(); ++current_pizza)
{
pizza_list.remove(current_pizza);
delete(current_pizza);
}
system("PAUSE");
return 0;
}
-
either that way
Code:
pizza_list.push_front(*pizza);
or you declare
Code:
list<cPizza*> pizza_list;
Kurt
-
Nope, there's no need to use pointers here at all. When you pass an instance of cPizza to push_back, a copy is made, so you can pass it to push_back as many times as you want and a different copy will be added each time.
I would completely remove this line of code:
Code:
cPizza *pizza, *current_pizza;
Then under your Add a pizza comment, I would create the pizza variable locally:
Code:
cPizza pizza(diameter, cost);
Finally, do not use a pointer to iterate through the list. The STL uses something called an iterator, that is occasionally implemented as a poiner, but often not. So your declaration of current_pizza should look like this:
Code:
list <cPizza>::iterator current_pizza;
There are plenty of other little things, like Stoned_Coder's recommendation to not declare variables until you are ready to initilaize and use them, but for the most part it is good.
-
Ok I'm getting there now I think :)
There seems to be a problem with the code that prints current_pizza->value(). (It says "passing `const cPizza' as `this' argument of `double cPizza::value()' discards qualifiers").
Also there is somthing wrong with the pizza_list.remove(*current_pizza); line.
Thanks for the help, I was getting pretty confused trying to switch over to c++ and also I wanted to make sure I started coding well.
Code:
#include <iostream>
#include <cstdlib>
#include <list>
using namespace std;
const double PIE = 3.14;
// Define the cPizza class
class cPizza
{
double radius, area;
public:
double value(void);
int diameter;
double cost;
cPizza(int diameter, double temp_cost);
};
// The contructor for cPizza
cPizza::cPizza(int temp_diameter, double temp_cost)
{
diameter = temp_diameter;
radius = diameter / 2;
area = (radius * radius) * PIE;
cost = temp_cost;
}
// Calculates the value for the pizza
double cPizza::value()
{
return area / cost;
}
int main(int argc, char *argv[])
{
cout << "Pizza calculator\n\n";
cout << "Size of pizza (Diameter in inches.): ";
int diameter;
cin >> diameter;
cout << "Cost of pizza (Pounds): ";
double cost;
cin >> cost;
// Add a pizza
cPizza pizza(diameter, cost);
list <cPizza> pizza_list;
pizza_list.push_front(pizza);
cout << "Size\t" << "Cost\t" << "Value\n";
list <cPizza> ::const_iterator current_pizza;
// Display all the pizza's
for(current_pizza = pizza_list.begin(); current_pizza != pizza_list.end(); ++current_pizza)
{
cout << current_pizza->diameter << "\t" << current_pizza->cost << "\t" << current_pizza->value() << "\n";
}
// Delete them all
for(current_pizza = pizza_list.begin(); current_pizza != pizza_list.end(); ++current_pizza)
{
pizza_list.remove(*current_pizza);
}
system("PAUSE");
return 0;
}
-
There is no need for any of the code in the Delete them all loop. This is one of the benefits of using local variables instead of dynamically allocated ones. Everything is cleaned up automatically.
You are using a const_iterator, which is a good choice, but if you use that you must make your cPizza class const-correct. That means that any function that doesn't change the visible state of the class data should be declared as const. Since value() only uses the member values, but doesn't change them, it should be a const function. That will allow you to call it from a const_iterator.
-
I changed the value function to "const double value(void);" and I get the same error as before but with const on the front.
-
Use
Code:
double value(void) const;
Kurt