5
\$\begingroup\$

This is a follow-up question to this: class-item-answer-to-one-of-the-unsolved-stack-overflow-qes.

As per the comments I received from @Ben Steffan, I have re-written the above classes as follows. However a few comments on it:

  1. Suggestion for getters and setters: I have tried to make setters but for getters, I have no idea, how they actually work for a pure abstract class, since we can't instantiate the class(and call the member function like object.getSomething()). I really would like to receive comment on this.
  2. I would like to const& always, whenever it's possible because, it will work for both lvalues and rvalues, which helps not to have extra coding for rvalues separately(just my point of view and habits). I am not saying that it should be always like that(of course there must be some cases where we should have either lvalue calls or rvalue calls.) But in this particular case(constructors and class methods), it would be okay to use. here is the complete code:

New Implementation

#include <iostream>
#include <string>
#include <unordered_map>
#include <memory>

/**************** Item: defined as a pure abstract class ************************ **/
class Item
{
protected:
    int m_code;
    int m_price;
    int m_quantity;
public:
    Item() = default;

    void setCode(const int& code)   {   m_code = code;  }
    void setPrice(const int& price)   {   m_price = price;  }
    void setQuantity(const int& quantity)   {   m_quantity = quantity;  }

    /*const int& getCode()const { return m_code;  }
    const int& getPrice()const { return m_price;  }
    const int& getQuantity()const { return m_quantity;  }*/

    virtual int calculateStockValue() = 0;
    virtual ~Item(){ };
};

/** ItemTable : inherits from Item, to store code - price - quantity to hash table **/
class ItemTable: public Item
{
private:
    std::unordered_map<int, std::pair<int,int>> m_itemTable;
public:
    ItemTable() = default;
    //(ii) to display the data members according to the item code
    void displayItem(const int& code)
    {
        if(m_itemTable.find(code) != m_itemTable.cend())
        {
            auto it = m_itemTable.find(code);
            std::cout << "Item code = " << it->first;
            std::cout << "\t price = " << it->second.first;
            std::cout << "\t quantity = " << it->second.second << std::endl;
        }
        else
            std::cout << "Invalid Item code!" << std::endl;
    }
    // (iii) appending Item to the list
    void appendItem(const int& code, const  int& price, const int& quantity)
    {
        //(i) input the data members
        setCode(code);
        setPrice(price);
        setQuantity(quantity);
        m_itemTable.emplace(m_code, std::make_pair(m_price, m_quantity));
    }
    // (iv) delete the Item from the list
    void deleteItem(const int& code)
    {
        if(m_itemTable.find(code) != m_itemTable.cend())
        {
            std::cout << "The item with code: " << code
                    << " has been deleted successfully!" <<std::endl;
            m_itemTable.erase(m_itemTable.find(code));
        }
        else
            std::cout << "Invalid Item code!" << std::endl;
    }
    // (v) total value of the stock
    virtual int calculateStockValue() override
    {
        int m_totalStock = 0;
        for(const auto &it:m_itemTable)
            m_totalStock += (it.second.first * it.second.second);

        return m_totalStock;
    }
    virtual ~ItemTable(){ };
};
/********************* main ******************************/
int main()
{
    int number;
    int code;
    std::unique_ptr<ItemTable> obj = std::make_unique<ItemTable>();

    std::cout << "Enter total number of Items : ";  std::cin >> number;

    for(int i = 0; i<number; ++i)
    {
        int code, price, quantity;
        std::cout << i+1 << ": Enter the Item code = "; std::cin >> code ;
        std::cout << i+1 << ": Enter the Item price = "; std::cin >> price ;
        std::cout << i+1 << ": Enter the Item quantity = "; std::cin >> quantity ;
        obj->appendItem(code, price, quantity);
    }

    std::cout << "Total Value of stock = " << obj->calculateStockValue() << std::endl;

    std::cout << "Enter code of the Item to be displayed: "; std::cin >> code;
    obj->displayItem(code);

    std::cout << "Enter code to delete the Item : ";         std::cin >> code;
    obj->deleteItem(code);

    std::cout << "Total Value of stock = " << obj->calculateStockValue() << std::endl;

    return 0;
}
\$\endgroup\$
4
\$\begingroup\$
class Item
{
protected:
    int m_code;
    int m_price;
    int m_quantity;
public:
    Item() = default;

Avoid protected data. If you've ever been told don't use global variables, this is essentially a smaller-scale version of the same problem.

You find yourself violating the first three design principles of SOLID.

  • Single Responsibility Principle - Coupling between classes gets tighter. Testing becomes more difficult.

  • Open/Closed Principle - The base class may need to make assumptions about the data member. Inheritors may be too flexible causing modification by extension.

  • Liskov Substitution Principle - protected data usually has some sort of invariance associated with them, otherwise those data members would have been public. By making them protected, any data invariant would not only have to be maintained by the base class, they would have to be maintained by the inheritors.

Constructors should create a fully initialized objects. The compiler-generated default constructor does not initialize the built-in types. Depending on the context in which Item is used, the values for your members could be 0 or could be uninitialized. In the uninitialized case, it is undefined behavior as to what the value references to when read. The simplest solution is to use in-class default member initialization.

struct Item {
private:
  int m_code {0};
  int m_price {0};
  int m_quantity {0};
public:
  Item() = default;

    void setCode(const int& code)   {   m_code = code;  }
    void setPrice(const int& price)   {   m_price = price;  }
    void setQuantity(const int& quantity)   {   m_quantity = quantity;  }

Pass cheaply-copied types by value. Nothing really beats the simplicity and safety of copying in this instance. It's also faster as it doesn't require the extra indirection on access.

Treat data like data. Treat class objects like class objects. Sometimes you need to simply store records in an aggregate. In this case, avoid trivial getters and setters. Sometimes you need class objects to reflect behavior and/or maintain invariants.

struct Item {
public:
  int m_code {0};
  int m_price {0};
  int m_quantity {0};
};

or

struct Item {
private:
  int m_code {0};
  int m_price {0};
  int m_quantity {0};
public:
  Item() = delete;
  Item(int code, int price, int quant = 0)
      : m_code{code}, m_price{price}, m_quantity{quant} {}
  // ...
};

    virtual ~Item(){ };

Abide by the Rule of Zero/Three/Five. If you define any of the special member functions (destructor, copy constructor, move constructor, copy assignment, move assignment), then define them all. Scott Myers takes this a step further and recommends you declare them explicitly and explicitly opt into the compiler-generated implementations.

    Item(const Item&) = default;
    Item& operator=(const Item&) = default;

    Item(Item&&) = default;
    Item& operator=(Item&&) = default;

    virtual ~Item() = default;
    virtual int calculateStockValue() = 0;

/**************** Item: defined as a pure abstract class ************************ **/

Don't say in comments what can be clearly stated in code. Compilers don't read comments. Comments are often less precise than code. Comments are not updated as often as code.

A pure abstract class has only abstract member functions and is intended to define an interface to be inherited by concrete classes. A way of forcing a contract between the class designer and users of that class as the users of the class must declare matching member functions for the class in order to compile. Item has both concrete member functions and data, which implies the class is an implementation and not an interface.

/** ItemTable : inherits from Item, to store code - price - quantity to hash table **/

Use class hierarchies to represent concepts with inherent hierarchical structure. Inheritance is the process of inheriting properties of objects by one class with another. The properties of Item are not represented through ItemTable.

Never inherit when composition is sufficient. ItemTable maintains a collection of Item. Don't use inheritance just to avoid locally defining variables.


        if(m_itemTable.find(code) != m_itemTable.cend())
        {
            auto it = m_itemTable.find(code);

Keep previously computed results around for operations that could be costly. The complexity of std::unordered_map::find() is constant on average, linear in the worst case.

        auto it = m_itemTable.find(code);
        if(it != m_itemTable.cend())
        {
            // ...

introduces if statements with scoped-initializers.

        if (auto it = m_itemTable.find(code); it != m_itemTable.cend())
        {
            // ...

        m_itemTable.emplace(m_code, std::make_pair(m_price, m_quantity));

From std::unordered_map::emplace():

Inserts a new element into the container constructed in-place with the given args if there is no element with the key in the container.

In the event of a item with a duplicate code, the new entry is simply discarded. Perhaps it would be better to merge the two records?


    std::unique_ptr<ItemTable> obj = std::make_unique<ItemTable>();

Prefer scoped objects. Don't heap-allocate unnecessarily.


    virtual int calculateStockValue() override { ... }

Virtual Functions should specify exactly one of virtual, override, or final. Use virtual when declaring a new virtual function. Use override when declaring an overrider.


    virtual ~ItemTable(){ };

Destructors in derived classes do not need to be specified as virtual. Derived destructors are implicitly virtual.


    std::cout << "Enter total number of Items : ";  std::cin >> number;

Prefer one statement per line.

Always check to make sure an operation succeeded.

    std::cout << "Enter total number of Items : ";  
    if (!(std::cin >> number)) {
        // Boom?
    }

    std::cout << "Total Value of stock = " << obj->calculateStockValue() << std::endl;

Don't use std::endl. std::endl is the equivalent of streaming '\n' followed by a std::flush. If you want to print an end-of-line character, prefer streaming '\n' as it explicitly states your intent and is shorter.


Suggestion for getters and setters: I have tried to make setters but for getters, I have no idea, how they actually work for a pure abstract class, since we can't instantiate the class(and call the member function like object.getSomething()). I really would like to receive comment on this.

You've already figured it out. If you want to access the members of an abstract class, have a concrete class inherit from it. The getters and setters work fine as long as the concrete class implements all of the virtual functions of the base.

I would like to const& always, whenever it's possible because, it will work for both lvalues and rvalues, which helps not to have extra coding for rvalues separately(just my point of view and habits). I am not saying that it should be always like that(of course there must be some cases where we should have either lvalue calls or rvalue calls.) But in this particular case(constructors and class methods), it would be okay to use.

Just think about this: One does not simply move a const object. By always const&'ing, you are forcing copies anywhere a move would be better.


Original Question Looking back at the original task, you didn't follow the first two requirements.

  1. The item list needed to be a linear array type.

  2. You are to take a capacity, meaning you can reserve space ahead of time. What should happen if \$n+1\$ elements are entered into the list? Should it continue to add elements, expanding its capacity? Should the capacity be fixed and the user informed there is no space?

The requirements are telling you to use a std::vector or similarly adapted container.

\$\endgroup\$
  • \$\begingroup\$ I disagree with your point on the rule of five. Defining the other special member functions only makes sense if they actually do something special.The reason that a destructor is defined here is not that the class would require any special behavior on destruction, but simply prevents UB from destruction through pointer of base class, so omitting the other members is fine, imo. \$\endgroup\$ – Ben Steffan Mar 25 '18 at 11:59

Your Answer

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