Object-oriented purchase data structure

Question

I wrote a code where customer can buy an item according to customer balance, price and quantity of item to be bought. Here is the code:

#include "bits/stdc++.h"
using namespace std;

class Item{
private:
string itemName;
string itemidNo;
int itemQuantity;
double itemPrice;
public:
Item(){
    itemPrice = 500;
    itemQuantity = 1;
}
//getters and setters
void setName(string name){itemName = name;}
void setId(string id){itemidNo = id;}
void setQuantity(int quantity){itemQuantity = quantity;}
void setPrice(double price){itemPrice = price;}

string getName(){return itemName;}
string getId(){return itemidNo;}
int getQuantity(){return itemQuantity;}
double getPrice(){return itemPrice;}

};

class customer {
private:
string Custname;
string CustidNo;
double Custbalance;
Item Custitem;
public:
customer(){
    Custbalance = 5000;
}
//getters and setters
void setName(string name){Custname = name;}
void setidNo(string id){CustidNo = id;}
void setbalance(double balance){Custbalance = balance;}

string getName(){return Custname;}
string getidNo(){return CustidNo;}
double getbalance(){return Custbalance;}

void print(){
    cout << Custitem.getName() << " " << Custitem.getId() << " " << Custitem.getQuantity() << " " << Custitem.getPrice() << "\n";
    Custbalance -= Custitem.getQuantity()*Custitem.getPrice();
    cout << Custbalance << "\n";
}

void buyItem(Item item){
    if(Custbalance < (item.getQuantity()*item.getPrice())){
        cout << "Insufficient Balance for " << item.getName() << "\n"; 
        return ;
    }
    if(item.getQuantity() < 1){
        cout << "order not valid";
        return;
    }
    Custitem = item;
    print();
}
};

int main(int argc, char const *argv[])
{
customer harsh;
harsh.setName("harsh");
harsh.setidNo("23");

Item item1;
item1.setName("mobile");
item1.setId("44");

harsh.buyItem(item1);

Item item2;
item2.setName("speakers");
item2.setId("F&D");
item2.setQuantity(2);
item2.setPrice(3000);

harsh.buyItem(item2);


return 0;
}

I am a bit confused as I think there is too much redundancy in my code, such as the getters and setters. How can I make my code more object-oriented?

Answer 1

Headers and namespaces

#include "bits/stdc++.h"

That's not a standard C++ header; prefer to use standard headers so that others can use your code.

using namespace std;

Bringing all names in from a namespace is problematic; namespace std particularly so. See Why is “using namespace std” considered bad practice?.

Naming

Choose a consistent naming convention and stick to it. A common one that's closest to the majority of your code is to use PascalCase for types, camelCase for members and snake_case for variables.

Class Base

A default-constructed Base will have default-constructed Id and Name members. It probably makes no sense to default-construct; I think it's better to explicitly declare a constructor:

public:
    Base(std::string id, std::string name)
      : id(std::move(id)), name(std::move(name))
    {}

Once initialized, there should be no reason to change the name or id, so we can remove the setters. The accessors can be made const, so we can call them on instances of const Base:

    std::string getName() const { return name; }
    std::string getId() const { return id; }

Class Item

This seems mis-named - it's more like an "order line" that specifies how many items and their price. Again, a default constructor seems wrong, although defaulting the quantity to 1 makes sense:

public:
    Item(std::string id, std::string name, double price, int quantity = 1)
      : Base(std::move(id), std::move(name)),
        itemPrice(price),
        itemQuantity(quantity)
    {
        if (itemQuantity <= 0)
            throw new std::domain_error("negative quantity");
        if (itemPrice <= 0)
            throw new std::domain_error("negative price");
    }

Whether it's reasonably to modify the price and quantity after construction really depends on your application, so I can't comment on that, but the getters should both be marked const, like the ones for id and name.

Also, I'll add the standard warning that floating-point is usually a poor choice for financial accounting - you'll avoid numeric rounding if you choose instead to represent currency amounts in integer values of the smallest denomination.

Class customer

Although customers may have names and identifiers, does that really make them the same kind of thing as items? Unless you have a use case where you may want to use them interchangeably, public inheritance isn't really what you need here. It's a common trap in OO design to assume that because two things are made from similar pieces that they must be similar things. In some cases, it really is mere coincidence. By way of analogy, chairs and dogs are both quadrupeds, but blindly choosing to treat them alike is a mistake that could turn around and bite you.

It is extremely kind of you to credit all your new customers with 5000 currency units in their accounts - but it might not be good business sense (even if they are Zimbabwean Third Dollars). I recommend 0. as a more sensible default.

If the customer object is intended to be an autonomous object, it should be responsible for crediting and debiting the account, rather than setting the amount; I'd expect something more like:

void deposit(Currency amount)
{
    balance += amount;
}
void withdraw(Currency amount)
{
    balance -= amount;
}
Currency balance() const { return amount; }

(Real implementations usually involve some locking, so that concurrent operations don't interfere with each other, but that's a whole review in itself)

Why can a customer buy only one Item? Usually, an order is composed of several items (each with a quantity) and refers to a customer (who may at a later date order more items - it's bad business to restrict them from doing that).

Something that looks really strange is this:

void print(){
    cout << Custitem.getName() << " " << Custitem.getId() << " " << Custitem.getQuantity() << " " << Custitem.getPrice() << "\n";
    Custbalance -= Custitem.getQuantity()*Custitem.getPrice();
    cout << Custbalance << "\n";
}

Why does the customer have to pay each time you print their order?

Usually, print() would look something like

std::ostream& print(std::ostream& o) const
{
    return o
      << Custitem.getName()
      << " " << Custitem.getId()
      << " " << Custitem.getQuantity()
      << " " << Custitem.getPrice() << "\n"
      << Custbalance << "\n";
}

and the debiting of the account would be a separate (non-const) operation.

Answer 2

So you have one class for customer and one class for item, with public getters and setters and only one other method buyItem.

First of all, you have inconsistent convention for capitalisation of identifiers: some classes and members are, some are not. You should choose a rule and consistently apply it, so that when we see one in your code, we immediately know whether it is a class or not.

But one of the OOP principle is that object are managed through methods, that ideally represent real use cases or business rules. Here only one looks like that: buyItem => this one must be public, other modifiers probably not.

When you look at the way objects are created, your code consistently creates an empty object (no parameters for the constructor) and then manually set some attributes => that means that you really should have a parameterized constructor, with default values:

class Item{
private:
    string itemName;
    string itemidNo;
    int itemQuantity;
    double itemPrice;

public:
    Item(string itemName, string itemidNo, int itemQuantity = 1, double itemPrice = 500){
        this.itemName = itemName;
        this.itemidNo = itemidNo;
        this.itemPrice = itemPrice ;
        this.itemQuantity = itemQuantity ;
    }

You can then create item more simply:

Item item1("mobile", "44");              // default values for quantity and price
Item item2("speakers", "F&D", 2, 3000);  // explicit values

and no longer need public setters (same for customer class).

You forgot the const declaration on getters when they do not modify the object:

int getQuantity() const {return itemQuantity;}

that will later allow to apply a getter on a const object.

Last but not least, you have a single Item object in customer class. It may or not be enough for your requirements, but that means that you only keep a copy of the last bought item.

Answer 3

after lot of messing around with code , I found a way to reduce the retundancy , since customer and item share two same attributes name and id so i made an interface for the corresponding. Here is my refactored code -

#include "bits/stdc++.h"
using namespace std;

class Base{
//item and name both have two common properties name and id
protected:
string Name;
string Id;

public:
void setName(string name){Name = name;}
void setId(string id){Id = id;}

string getName(){return Name;}
string getId(){return Id;}
};

class Item : public Base{
private:
int itemQuantity;
double itemPrice;
public:
Item(){
    itemPrice = 500;
    itemQuantity = 1;
}
//getters and setters
void setQuantity(int quantity){itemQuantity = quantity;}
void setPrice(double price){itemPrice = price;}

int getQuantity(){return itemQuantity;}
double getPrice(){return itemPrice;}

};

class customer : public Base{
private:
double Custbalance;
Item Custitem;
public:
customer(){
    Custbalance = 5000;
}
//getters and setters
void setbalance(double balance){Custbalance = balance;}

double getbalance(){return Custbalance;}

void print(){
    cout << Custitem.getName() << " " << Custitem.getId() << " " << Custitem.getQuantity() << " " << Custitem.getPrice() << "\n";
    Custbalance -= Custitem.getQuantity()*Custitem.getPrice();
    cout << Custbalance << "\n";
}

void buyItem(Item item){
    if(Custbalance < (item.getQuantity()*item.getPrice())){
        cout << "Insufficient Balance for " << item.getName() << "\n"; 
        return ;
    }
    if(item.getQuantity() < 1){
        cout << "order not valid";
        return;
    }
    Custitem = item;
    print();
}
};

int main(int argc, char const *argv[])
{
customer harsh;
harsh.setName("harsh");
harsh.setId("23");

Item item1;
item1.setName("mobile");
item1.setId("44");

harsh.buyItem(item1);

Item item2;
item2.setName("speakers");
item2.setId("F&D");
item2.setQuantity(2);
item2.setPrice(1000);

harsh.buyItem(item2);


return 0;
}