Thread: Couple of questions

  1. #1
    Registered User
    Join Date
    May 2004
    Posts
    114

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

  2. #2
    Unregistered User
    Join Date
    Sep 2005
    Location
    Antarctica
    Posts
    341
    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;

  3. #3
    Registered User
    Join Date
    Aug 2005
    Posts
    1,267
    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;

  4. #4
    Registered User
    Join Date
    Jan 2005
    Posts
    7,366
    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.

  5. #5
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,817
    Code:
    #include <stdlib.h>
    The above version of that particular header file is deprecated, you should prefer this version of the header file:

    Code:
    #include <cstdlib>
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  6. #6
    Registered User
    Join Date
    Mar 2002
    Posts
    1,595
    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.
    You're only born perfect.

  7. #7
    Skunkmeister Stoned_Coder's Avatar
    Join Date
    Aug 2001
    Posts
    2,572
    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.
    Free the weed!! Class B to class C is not good enough!!
    And the FAQ is here :- http://faq.cprogramming.com/cgi-bin/smartfaq.cgi

  8. #8
    Registered User
    Join Date
    May 2004
    Posts
    114
    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;
    }
    Last edited by kzar; 10-07-2005 at 03:27 PM.

  9. #9
    Registered User
    Join Date
    Aug 2005
    Location
    Austria
    Posts
    1,990
    either that way
    Code:
        pizza_list.push_front(*pizza);
    or you declare
    Code:
    list<cPizza*> pizza_list;
    Kurt

  10. #10
    Registered User
    Join Date
    Jan 2005
    Posts
    7,366
    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.

  11. #11
    Registered User
    Join Date
    May 2004
    Posts
    114
    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;
    }

  12. #12
    Registered User
    Join Date
    Jan 2005
    Posts
    7,366
    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.

  13. #13
    Registered User
    Join Date
    May 2004
    Posts
    114
    I changed the value function to "const double value(void);" and I get the same error as before but with const on the front.

  14. #14
    Registered User
    Join Date
    Aug 2005
    Location
    Austria
    Posts
    1,990
    Use
    Code:
    double value(void) const;
    Kurt

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Couple of Questions About Classes
    By bengreenwood in forum C++ Programming
    Replies: 3
    Last Post: 05-20-2009, 02:50 PM
  2. Couple of Questions
    By toonlover in forum Windows Programming
    Replies: 10
    Last Post: 07-14-2006, 01:04 AM
  3. Couple of simple directdraw questions.
    By Deo in forum Game Programming
    Replies: 3
    Last Post: 05-25-2005, 07:55 AM
  4. A couple of questions that someone might know the answer to...
    By Finchie_88 in forum A Brief History of Cprogramming.com
    Replies: 6
    Last Post: 04-15-2005, 08:26 AM
  5. Studying for Final, Couple Questions...
    By stewade in forum C Programming
    Replies: 4
    Last Post: 05-10-2004, 05:02 PM