3
\$\begingroup\$

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?

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Oops, some of these comments address the modified code posted as answer. I hope it's still useful! \$\endgroup\$ – Toby Speight Aug 22 '17 at 13:51
  • \$\begingroup\$ if u look closely in the code, i only call the function print() when the customer buys something \$\endgroup\$ – gsdf Aug 22 '17 at 15:12
  • \$\begingroup\$ Yes, but it's not clear to anybody else that they mustn't call print() at any other time (e.g. when reviewing past orders). The code at present isn't (yet) doing anything wrong, but it's misleading, and misleading code gives rise to avoidable errors. \$\endgroup\$ – Toby Speight Aug 22 '17 at 15:37
  • \$\begingroup\$ ok then i should make it private \$\endgroup\$ – gsdf Aug 22 '17 at 15:46
  • 1
    \$\begingroup\$ @gsdf: IMHO, print should not be made private, but it should not change the object and should then be marked const as the getters. \$\endgroup\$ – Serge Ballesta Aug 22 '17 at 15:48
2
\$\begingroup\$

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.

\$\endgroup\$
-1
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ You might be focusing too much on the perceived commonality here. While in your model both have a name and an id, if you look to the real word they are not really the same, an id for people would be something like a Social Security number (in the USA). For items that might be a UPC code, this is not the same. Just because fields are common doesn't necessarily mean that you want to derive from a common base. \$\endgroup\$ – Harald Scheirich Aug 22 '17 at 14:04
  • \$\begingroup\$ ^ second this. These properties happened to be named the same, but they do not represent the same thing. Also - Base is a terrible name even if this was the case. Things named generically like base usually indicate your model is not quite right \$\endgroup\$ – Milney Aug 22 '17 at 14:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.