Item categories

Question

This is my second day into C++ and I wanted to get a small review from someone experienced with the language. I'm coming from Python so some stuff seems kind of weird. My plan is to build a little "web store" with this.

#include <string>
#include <vector>
#include <iostream>
#include <initializer_list>

enum class ItemCategory{
    FOOD,
    CLOTHES
};

class Item{
    private:
        int id;
        std::string item_name;
        double item_price;
        bool item_available;
        int stock;
        ItemCategory item_category;

    public:
        static int id_ref;
        Item(){}
        Item(std::string _name, double _price, ItemCategory _category);
        Item(std::string _name, double _price, int _stock, ItemCategory _category);
        Item& operator=(const Item& i);
        //Getters
        const int getId(){return id;}
        const std::string getName(){return item_name;}
        const double getPrice(){return item_price;}
        const bool getAvailable(){return item_available;}
        const int getStock(){return stock;}
        const ItemCategory getCategory(){return item_category;}
        const std::string getCategoryString();
        //Setters
        void setName(std::string n){item_name = n;}
        void setPrice(double p){item_price = p;}
        void setAvaiable(bool b){item_available = b;}
        void setStock(int s){stock = s;}
        void setCategory(ItemCategory ic){item_category = ic;}

};

//Copy operator
Item& Item::operator=(const Item& i){
    return *this;
}

//Keep track of all Items created to index them.
int Item::id_ref = 0; 
const std::string Item::getCategoryString(){
    switch(item_category){
        case (ItemCategory::FOOD):
            return "Food";
        case (ItemCategory::CLOTHES):
            return "Clothes";
    }
}

Item::Item(std::string _name, double _price, ItemCategory _category):
        id{Item::id_ref++}, item_name{_name}, item_price{_price}, 
        item_available{true}, stock{1}, item_category{_category}{}

Item::Item(std::string _name, double _price, int _stock, ItemCategory _category):
    id{Item::id_ref++}, item_name{_name}, item_price{_price}, 
    item_available{true}, stock{_stock}, item_category{_category}{}

class Cart{
    private:
        std::vector<Item> customer_cart;

    public:
        Cart(){}//Empty cart.
        Cart(std::initializer_list<Item> lst):customer_cart{lst}{}
        void push(Item i){customer_cart.push_back(i);}
        void printAllItems();
        //TODO: add range checking to subscription
        Item& operator[](int i){return customer_cart[i];}


};

void Cart::printAllItems(){
    std::cout << "Id   Name      Category     Price    Stock" << std::endl; 
    for(auto i:customer_cart){
        std::cout << i.getId() << "   " << i.getName() << "      " << i.getCategoryString()                << "         " << i.getPrice() << "      " << i.getStock() << std::endl;
    }
}

class Customer{
    private:
        int id;
        std::string customer_name;
        std::string customer_mail;
        std::string customer_password; //authentication
        Cart cart;

    public:
        static int id_ref;
        Customer(std::string _name, std::string _mail, std::string _password);
        Customer(std::string _name, std::string _mail, std::string _password, 
                 std::initializer_list<Item> lst);

        //Getters
        const std::string getName(){return customer_name;}
        const std::string getMail(){return customer_mail;}
        const std::string getPassword(){return customer_password;}
        const Cart& getCart(){return cart;}

        //Setters
        void setName(std::string n){customer_name = n;}
        void setMail(std::string m){customer_mail = m;}
        void setPassword(std::string p){customer_password = p;}

        //Utility
        void showAllItems(){cart.printAllItems();}

};
int Customer::id_ref = 0;

Customer::Customer(std::string _name, std::string _mail, std::string _password):
    id{Item::id_ref++},  customer_name{_name}, customer_mail{_mail}, customer_password{_password},
    cart(){}

Customer::Customer(std::string _name, std::string _mail, std::string _password, 
                   std::initializer_list<Item> lst):
    id{Item::id_ref++},  customer_name{_name}, customer_mail{_mail}, customer_password{_password},
    cart(lst){}

This is how I would use it:

#include "sales.h"
#include <iostream>

using namespace std;

int main(){
    Item i1("apple", 20, ItemCategory::FOOD);
    Item i2("shirt", 45, ItemCategory::CLOTHES);

    Customer c("randomName", "really_cool@gmail.com", "whatever");
    cout << c.getName() << endl;
    Cart cart = c.getCart();
    cart.push(i1);
    cart.printAllItems();

    cout << endl;

    //Constructor with init_list
    Customer b("anotherName", "cool_email@gmail.com", "testpass", {i1, i2});
    b.showAllItems();

}

Answer 1

Here are some small remarks:

1. You can combine both constructors (and remove the empty one which is kind of useless) by reordering stock as the last argument and provide an default value. Also constify input that you do not change and pass by reference when needed.

   Item(const std::string &name, 
        const double price, 
        const ItemCategory category, 
        const int stock = 0);
   

2. Setters and getters. Does it make sense to change the name or the category of an item? If not you should remove those setters.

3. Availability. Is there another reason rather than out of stock. If not you should remove that variable and just check for stock==0

4. I think you misunderstood what a copy operator does. I copies the data from the provided element into this one. To elaborate more, this code is wrong:

   //Copy operator
   Item& Item::operator=(const Item& i){
       return *this;
   }
   

   This simply returns your current item without assigning the data from i to it. Have a look at how this is used (reference). Basically you have to copy the data from i to your object and then return the object.

5. Your Cart stores a copy of the item which seems to mess with your ref counting. So why not pass a pointer Item* instead? So change your class to

   class Cart{
   private:
       std::vector<Item*> customer_cart;
   
   public:
       Cart(std::initializer_list<Item> lst = {})
       : customer_cart{lst}{}
       void push(const Item &item){customer_cart.push_back(&item);}
       void printAllItems();
       //TODO: add range checking to subscription
       Item& operator[](int index){return *customer_cart[index];}
   

   };

6. Customer id seems like it should be const.

7. You can use a default empty list for the constructors of the customer too.

Answer 2

One thing I saw immediately that the other answer didn't bring up.

C++ has pass-by-value semantics. Which means that when you pass an argument or return values (without & or *) you will create a copy of the object. If the object is complex, this can take some time and be wasteful if all you want to do is to look at the object. Under some circumstances the compiler may elide the copies, unless you're sure of when the compiler is allowed to do this I recommend that you pass by reference or pointer (& or *).

Here is a brief outline:

std::string foo(std::string x){ return x; }

// Here "bar" is passed as argument to an un-named temporary std::string
// namely to the constructor std::string(const char*). This temporary
// becomes 'x' in the function body. When 'x' is returned, another temporary
// is created and 'x' is copied into this and 'length()' is called on the
// temporary. This has to be done because 'x' doesn't exist outside of the
// function body. 
foo("bar").length();

std::string y = "bar"; // constructor 
y = foo(y); // y is copied to x, x is copied to y. (Bar RVO)

This will create a copy of the argument (rules changed slightly in C++11 but worry about that later) when you call the function and store it into the named variable x. When you return x the compiler generates another copy and returns that. Now all modern compilers will do Return Value Optimization (RVO) and in most cases the copy when returning will be optimized out.

const std::string& foo(const std::string& x){ return x; }

// Here a temporary std::string is created from "bar" again and
// now the 'x' refers to the memory of the temporary. The address of the
// temporary is returned, because the temporary was created outside of
// the function block, it is still alive until the call to 'length()' is
// made. 
foo("bar").length();

std::string x = foo("bar"); // Copy is made on assignment, not return.

std::string y = "foo";
y = foo(y); // no copies are made. 

This will take the argument x "by reference" meaning that x will refer to the memory of the object that was used as an argument, hence no copy is made. Because we used const we tell the compiler that we are only intending to call const declared methods on this class, methods which will not change the internal state of the class. I.e. we intend to take the argument as "read-only". Notice that we also return by const reference, this guarantees that we will not make a copy upon return, but a copy is be made when the return value is assigned to another variable.

As I hinted to earlier, in C++11 the rules changed slightly, the compiler is now able to elide copies much more efficiently and there are cases where passing by value is faster. But until you get a grasp of when and where this is true, it is safer to pass by const reference by default for non trivial types.

See: https://en.wikipedia.org/wiki/Reference_(C%2B%2B) And: https://stackoverflow.com/questions/3967177/when-to-use-const-and-const-reference-in-function-args