HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Item categories

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
itemcategoriesstackoverflow

Problem

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
#include
#include
#include

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 _na

Solution

Here are some small remarks:

-
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);


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

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

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

-
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 customer_cart;

public:
    Cart(std::initializer_list 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];}


};

-
Customer id seems like it should be const.

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

Code Snippets

Item(const std::string &name, 
     const double price, 
     const ItemCategory category, 
     const int stock = 0);
//Copy operator
Item& Item::operator=(const Item& i){
    return *this;
}
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];}

Context

StackExchange Code Review Q#144045, answer score: 3

Revisions (0)

No revisions yet.