4
\$\begingroup\$

I had to design a very simple e-shop in C++ but I don't know if my design is the correct one. I mean in sense of re-usability. How can I find out if I have done a good design and how can I further optimize the design? The task as a simple e-shop where someone could see the products, order a product and define its characteristics. And in the end to print out the result of the order. I am posting the whole code.

Products.h

#pragma once

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

// Create an Interface for Product Objects
class IProducts
{
public:
    // Virtual Function to get the name of the product implementing the interface
     virtual  std::string getProductName()  = 0;
    // Virtual Function to Display  the names of all components of a class implementing the interface
    virtual void DisplayComponents() = 0;
    // Virtual Function to display the values of the components of a class implementing the interface 
    virtual void Display() = 0;
    // Virtual Function to set the components to desired values 
    virtual void setAttributes() = 0;
};

// Concretion of Product Interface
class PC_Towers : public IProducts
{
public:
    // Function to set the member variables of the class
    void setAttributes ()
    {

            std::cout << "Please enter Memory size for PC_Tower in GB : ";
            // MAke sure that the input in numeric
            while(!(std::cin >> this->Memory))
            { 
                std::cout << "All input's must be numeric " << std::endl;

                break;
            }

            std::cout << "Please enter CPU size for PC_Tower in GHz : ";
            while (!(std::cin >> this->CPU))
            {

                std::cout << "All input's must be numeric " << std::endl;
                break;
            };




    }
    // Function to get the Name of the product
    std::string getProductName() { return this->productName; }
    // Function to display the names of the components of the class
    void DisplayComponents() { std::cout<<"The Tower is composed from : 1) Memory  2) CPU " << std::endl; }
    // Function to display the values of the member variables
    void Display()
        {
        std::cout << "Your Tower has a Memory of " << this->Memory << " GB and a CPU of " << this->CPU << " GHZ" << std::endl;


        }


private:
    double Memory;
    double CPU;
    const std::string productName = "PC_Tower";
};

// Another concrition on the IProduct interface the same as the one before
class PC_Screen : public IProducts
{
public:
    void setAttributes () 
    {


        std::cout << "Please enter size of your Screen in inches: " ;
        while (!(std::cin >> this->Size_inch))
        {
            std::cout << "All input's must be numeric " << std::endl;
            break;


        }



    }
    std::string getProductName() { return this->productName; }
    void DisplayComponents() { std::cout << "The screen is composed from a screen measured in inches " << std::endl; }
    void Display()
    {
        std::cout << "Your screen is " << this->Size_inch << " inches " << std::endl;


    }

private:
    double Size_inch;
    const std::string productName = "PC_Screen";
};
// Concrition of IProducts
class Personal_Computer : public IProducts
{
public:
    // Function to set the attributes of the member variable. In this case the function works as a decorator
    // arround the setAttributes of the IProduct adding functionalities to it
    void setAttributes() 
    {
        Tower.setAttributes();
        Screen.setAttributes();

        std::cout << " Please enter size of your HardDics in GB : " ;
        while (!(std::cin >> this->HardDisc))
        {
            std::cout << "All input's must be numeric " << std::endl;
            break;
        }



    }
    std::string getProductName() { return this->productName; }
    // Decorate the DisplayComponents() and add functionalities
    void DisplayComponents() 
    { 
        std::cout << "Personal Computer is composed from: 1) Tower 2) PC Screen 3) Hard Disc" << std::endl;
        Tower.DisplayComponents();
        Screen.DisplayComponents();

    }
    // Decorate the Display() and add functionalities
    void Display()
    {
        Tower.Display();
        Screen.Display();
        std::cout << "Your Hard Disc has size : " << this->HardDisc << " GB " << std::endl;


    }


private:
    PC_Towers Tower;
    PC_Screen Screen;
    double HardDisc;
    const std::string productName = "Personal_Computer";
};

// Concretion of Iproduct
class Work_Station : public IProducts
{
public:
    void setAttributes()
    {
        Computer.setAttributes();

        std::cout << "Please Enter your Operating System " ;
        while (!(std::cin >> this->OperatingSystem))
        {
            std::cout << "Operating system must be string " << std::endl;
            break;
        }


    }
    std::string getProductName() { return this->productName; }
    void DisplayComponents()
    {
        std::cout << "Work station is composed from : 1) Personal computer 2) Operating System (Linux or Windows) " << std::endl;
        Computer.DisplayComponents();
    }
    void Display()
    {
        Computer.Display();
        std::cout << "Your Operating System is :" << this->OperatingSystem << std::endl;

    }

private:
    Personal_Computer Computer;
    std::string OperatingSystem;
    std::string productName = "WorkStation";
};

// Interface of Factory to create IProducts
class IProductFactory
{
public:
    virtual IProducts* createProduct(std::string myProduct) = 0;

};
// Concretion of Interface for IProduct creation. This Factory produces IProducts based on the an string input 
// to the function ( like a user input)
class UserInputFactoryProduct : public IProductFactory
{
public:

    IProducts* createProduct(std::string myProduct)
    {
        IProducts* product;
        if (myProduct == "PC_Tower")
            product = new PC_Towers;
        else if (myProduct == "PC_Screen")
            product = new PC_Screen;
        else if (myProduct == "Personal_Computer")
            product = new Personal_Computer;
        else if (myProduct == "WorkStation")
            product = new Work_Station;
        else
            product = nullptr;

        return product;

    }
    // Function to get the product member variable

};

// Class e-shop to add and display all the products of the shop
class e_shop
{
public:
    // Function to add products to the shop
    void addProduct(IProducts* newProduct) { this->allProducts.push_back(newProduct); }
    // Function to display all the products of the shop
    void desplayAllProducts()
    {

        for (int i = 0  ; i < allProducts.size()  ; i++)
            std::cout << allProducts.at(i)->getProductName() << std::endl;
    }
private:
    // vector to keep all the products of the shop
    std::vector< IProducts* > allProducts;
};

And this is main

#include "Products.h"

int main()
{
    // create some products
    IProducts* Product1 = new PC_Towers;
    IProducts* Product2 = new PC_Screen;
    IProducts* Product3 = new Personal_Computer;
    IProducts* Product4 = new Work_Station;
    // create an e-shop and add the products created
    e_shop myEshop;
    myEshop.addProduct(Product1);
    myEshop.addProduct(Product2);
    myEshop.addProduct(Product3);
    myEshop.addProduct(Product4);
    myEshop.desplayAllProducts();

    std::string choosedProduct;
    std::cout << std::endl;
    IProducts* myProduct = nullptr;
    UserInputFactoryProduct ProductFactory;

    // choose a product and use factory to create the object based on the user input
    while (myProduct == nullptr)
    {
        std::cout << "Chose one of the above products : ";
        std::cin >> choosedProduct;

        myProduct = ProductFactory.createProduct(choosedProduct);

    } ;

    // display all the attributes of the product
    myProduct->DisplayComponents();
    // let the user to add values to components
    myProduct->setAttributes();
    // display the product ith the values of the user
    myProduct->Display();

    system("pause");
}

```
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Edward May 13 '19 at 12:50
  • \$\begingroup\$ It looks like you have two files here, one is products.h ans one is main.cpp but it is all one code block. Is it possible for you to split the code block into 2 code blocks with names before each code block? \$\endgroup\$ – pacmaninbw May 13 '19 at 13:07
  • 1
    \$\begingroup\$ @pacmaninbw Yes i edit it so now it is 2 code blocks \$\endgroup\$ – Aris Koutsoukis May 13 '19 at 13:11
  • \$\begingroup\$ Which C++ standard are you targeting? \$\endgroup\$ – JVApen May 13 '19 at 18:27
  • 1
    \$\begingroup\$ @ArisKoutsoukis: IProducts* Product1 = new PC_Towers; The trouble is this means you have a RAW pointer. RAW pointers harc back to C++03 before modern memory management techniques were introduced into the language. This has affected your design void addProduct(IProducts* newProduct) were you are accepting a RAW pointer (this is bad because it is unclear if the interface is accepting ownership). Use std::make_unique() or std::make_shared() for dynamic objects as these return smart pointers. \$\endgroup\$ – Martin York May 15 '19 at 16:09
5
\$\begingroup\$

Don't Ignore Warning Messages
This line in e_shop.desplayAllProducts() generates a warning message:

    for (int i = 0; i < allProducts.size(); i++)

The warning is that there is a type miss-match between the variable i and allProducts.size(). To remove this warning message i should be declared as type size_t. All STL container classes return size_t from the size function. The type size_t is unsigned rather than signed, integers are signed.

It might be better to use a ranged for loop using iterators rather than an index for loop:

    for (auto i : allProducts) {
        std::cout << i->getProductName() << std::endl;
    }

This may improve performance as well.

Reusability and Maintainability
It might be better to put each class in it's own file. While several of the classes depend on the interface (abstract class) IProducts, none of the classes depend on the other classes in the header file. The header file Products.h can include each of the product class files.

The productName string should be declared as a protected variable in the abstract class IProducts and rather than defining std::string getProductName() as an abstract function in IProducts create the full declaration of the function.

class IProducts
{
public:
    std::string getProductName() { return productName; }  // Note: no `this` keyword.
    // Virtual Function to Display  the names of all components of a class implementing the interface
    virtual void DisplayComponents() = 0;
    // Virtual Function to display the values of the components of a class implementing the interface 
    virtual void Display() = 0;
    // Virtual Function to set the components to desired values 
    virtual void setAttributes() = 0;
protected:
    std::string productName;
};

Have a constructor in each of the classes that inherits from IProducts that initializes productName to the proper value. The constructor should also initialize each of the private variables to a default value. Some of the classes might require destructors as well, they can always be set to the default constructor if they don't have special functions such as closing files.

Unlike some other languages such as PHP, the this keyword is not generally required in C++ and is generally not used. There may be certain special cases where it is required.

It might be better if each class had a .cpp file that contained the functions and only the class declaration was in the header file. This will improve compile times and not require all file to rebuild when changes are made in the executable code.

Portability
While

#pragma once

is widely supported, it is not part of the C or C++ standard and some C++ compilers may report a compilation error or warning message. This stackoverflow question also discusses the user of #pragma once. It might be better to use an explicit include guard.

#ifndef GRANDPARENT_H
#define GRANDPARENT_H

struct foo {
    int member;
};

#endif /* GRANDPARENT_H */

Performance
Generally the code will perform better if \n is used over std::endl. std::endl performs a file flush to the output stream and this adds time in each execution, if you use std::endl do it outside a loop after the loop has completed.

\$\endgroup\$
6
\$\begingroup\$

If I look at your code, it looks like you are programming in 98. C++ has evolved a lot, C++11 is the least to accept, C++17 should be the standard for this time.

getProductName could/should be a const method. Most likely, others can as well.

Looking at the PC_Towers class, I would expect it to be a final class.

I'm not in favor of the setAttributes, it doesn't belong in this class. Do you intend to add as well the logic to read it from file, database, json, XML ... to it? Best to separate it and use a kind of factory pattern. Looking closer: while (cond) { ... break; } looks a lot like an if-statement. How about using it to have less confusion?

From a class design, why would you create the same string for every instance. You could make it a static or simply inline it in your function call.

Looks like you are using a factory, let's use some abstractions here. Though, ignoring those, you duplicated the strings. One small typo, big consequences.

Looking at the factory, it still does naked new. Use std::make_unique as it prevents memory leaks you have in your program.

desplayAllProducts (typo?) looks like it can benefit from a range based for-loop. Oh, and don't use .at on a vector if you always know you index within bounds.

\$\endgroup\$
  • \$\begingroup\$ My main concern was the design and not the implementation. Of course if I was aiming for good implementation it would look much different with of course smart pointers and many other modern c++ features. \$\endgroup\$ – Aris Koutsoukis May 13 '19 at 20:15
0
\$\begingroup\$

Because I see that i get a lot of comments about the Row pointers , i changed it using smart pointers and more c++ 11. I am posting the hole code if anyone would like to use it. I made some of the changes proposed but not all of the proposed for example I was not able to make a factory for the setAttributes. And if anyone could give me a hint how to get rid of the if statements in the factory for the production of the IProducts i would appreciate it. And of course any other advice's would be welcome

User.h

#pragma once
#include"Products.h"

class IUser 
{
public:
   IUser(const std::string myName, const double myPassword) { name = myName, password = myPassword; }
   const std::string getName() const
   {
       return name;
   }

   const double getPassword() const
   {
       return password;
   }
protected:
   std::string name;
   double password;
};

class Client : public IUser
{
public:
   Client(const std::string myName, double passWord) :IUser(myName, passWord) {};
   void buyProduct(std::shared_ptr<IProducts> currentProduct) { boughtProducts.push_back(currentProduct); }
   void checkOut() {
       for (size_t i = 0; i < boughtProducts.size(); ++i)
       { 
           std::cout << "Your " << i + 1 << " bought product is " << boughtProducts[i]->getProductName() << " with the above charecteristics " << std::endl;
           boughtProducts[i]->Display();
       }
   }
private:
   std::vector<std::shared_ptr<IProducts>> boughtProducts;

};

Products.h

#pragma once

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

// Create an Interface for Product Objects
class IProducts
{
public:
    // Virtual Function to get the name of the product implementing the interface
     virtual   const std::string getProductName() const  = 0;
    // Virtual Function to Display  the names of all components of a class implementing the interface
    virtual void DisplayComponents() = 0;
    // Virtual Function to display the values of the components of a class implementing the interface 
    virtual void Display() = 0;
    // Virtual Function to set the components to desired values 
    virtual void setAttributes() = 0;
};

// Concretion of Product Interface
class PC_Towers final : public IProducts
{
public:
    // Function to set the member variables of the class
    void setAttributes ()
    {

            std::cout << "Please enter Memory size for PC_Tower in GB : ";
            // MAke sure that the input in numeric
            while(!(std::cin >> this->Memory))
            { 
                std::cout << "All input's must be numeric " << std::endl;

                break;
            }

            std::cout << "Please enter CPU size for PC_Tower in GHz : ";
            while (!(std::cin >> this->CPU))
            {

                std::cout << "All input's must be numeric " << std::endl;
                break;
            };




    }
    // Function to get the Name of the product
    const std::string getProductName() const { return this->productName; }
    // Function to display the names of the components of the class
    void DisplayComponents() { std::cout<<"The Tower is composed from : 1) Memory  2) CPU " << std::endl; }
    // Function to display the values of the member variables
    void Display()
        {
        std::cout << "Your Tower has a Memory of " << this->Memory << " GB and a CPU of " << this->CPU << " GHZ" << std::endl;


        }


private:
    double Memory;
    double CPU;
    const std::string productName = "PC_Tower";
};

// Another concrition on the IProduct interface the same as the one before
class PC_Screen : public IProducts
{
public:
    void setAttributes () 
    {


        std::cout << "Please enter size of your Screen in inches: " ;
        while (!(std::cin >> this->Size_inch))
        {
            std::cout << "All input's must be numeric " << std::endl;
            break;


        }



    }
    const std::string getProductName() const { return this->productName; }
    void DisplayComponents() { std::cout << "The screen is composed from a screen measured in inches " << std::endl; }
    void Display()
    {
        std::cout << "Your screen is " << this->Size_inch << " inches " << std::endl;


    }

private:
    double Size_inch;
    const std::string productName = "PC_Screen";
};
// Concrition of IProducts
class Personal_Computer : public IProducts
{
public:
    // Function to set the attributes of the member variable. In this case the function works as a decorator
    // arround the setAttributes of the IProduct adding functionalities to it
    void setAttributes() 
    {
        Tower.setAttributes();
        Screen.setAttributes();

        std::cout << " Please enter size of your HardDics in GB : " ;
        while (!(std::cin >> this->HardDisc))
        {
            std::cout << "All input's must be numeric " << std::endl;
            break;
        }



    }
    const std::string getProductName() const { return this->productName; }
    // Decorate the DisplayComponents() and add functionalities
    void DisplayComponents() 
    { 
        std::cout << "Personal Computer is composed from: 1) Tower 2) PC Screen 3) Hard Disc" << std::endl;
        Tower.DisplayComponents();
        Screen.DisplayComponents();

    }
    // Decorate the Display() and add functionalities
    void Display()
    {
        Tower.Display();
        Screen.Display();
        std::cout << "Your Hard Disc has size : " << this->HardDisc << " GB " << std::endl;


    }


private:
    PC_Towers Tower;
    PC_Screen Screen;
    double HardDisc;
    const std::string productName = "Personal_Computer";
};

// Concretion of Iproduct
class Work_Station : public IProducts
{
public:
    void setAttributes()
    {
        Computer.setAttributes();

        std::cout << "Please Enter your Operating System " ;
        while (!(std::cin >> this->OperatingSystem))
        {
            std::cout << "Operating system must be string " << std::endl;
            break;
        }


    }
    const std::string getProductName() const  { return this->productName; }
    void DisplayComponents()
    {
        std::cout << "Work station is composed from : 1) Personal computer 2) Operating System (Linux or Windows) " << std::endl;
        Computer.DisplayComponents();
    }
    void Display()
    {
        Computer.Display();
        std::cout << "Your Operating System is :" << this->OperatingSystem << std::endl;

    }

private:
    Personal_Computer Computer;
    std::string OperatingSystem;
    std::string productName = "WorkStation";
};



ProductsFactory.h

#pragma once
#include"Products.h"

class IProductFactory
{
public:
    virtual std::shared_ptr<IProducts> createProduct(std::string) = 0;

};
// Concretion of Interface for IProduct creation. This Factory produces IProducts based on the an string input 
// to the function ( like a user input)
class UserInputFactoryProduct : public IProductFactory
{
public:

    std::shared_ptr<IProducts> createProduct(std::string myProduct)
    {
        std::shared_ptr<IProducts> product;
        if (myProduct == "PC_Tower")
            product = std::make_shared<PC_Towers>();
        else if (myProduct == "PC_Screen")
            product = std::make_shared<PC_Screen>();
        else if (myProduct == "Personal_Computer")
            product = std::make_shared<Personal_Computer>();
        else if (myProduct == "WorkStation")
            product = std::make_shared<Work_Station>();
        else
            product = nullptr;

        return product;

    }


};


e_shop.h

#pragma once
#include"Products.h"

// Class e-shop to add and display all the products of the shop
class e_shop
{
public:
    // Function to add products to the shop
    void addProduct(std::shared_ptr<IProducts>newProduct) { this->allProducts.push_back(newProduct); }
    // Function to display all the products of the shop
    void desplayAllProducts()
    {

        for (auto e:allProducts)
            std::cout << e->getProductName() << std::endl;
    }
private:
    // vector to keep all the products of the shop
    std::vector< std::shared_ptr<IProducts> > allProducts;
};

main.cpp

#include "Products.h"
#include "e_shop.h"
#include"ProductsFactory.h"
#include "User.h"
int main()
{
    Client first("Aris", 12345);
    // create some products
    std::shared_ptr< IProducts > Product1 = std::make_shared<PC_Towers>();
    std::shared_ptr< IProducts > Product2 = std::make_shared<PC_Screen>();
    std::shared_ptr< IProducts > Product3 = std::make_shared<Personal_Computer>();
    std::shared_ptr< IProducts > Product4 = std::make_shared<Work_Station>();
    // create an e-shop and add the products created
    e_shop myEshop;
    myEshop.addProduct(Product1);
    myEshop.addProduct(Product2);
    myEshop.addProduct(Product3);
    myEshop.addProduct(Product4);
    myEshop.desplayAllProducts();
    std::string finish;

    while(finish != "N")
    { 
    std::string choosedProduct;
    std::cout << std::endl;
    std::shared_ptr<IProducts> myProduct = nullptr;
    UserInputFactoryProduct ProductFactory;

    // choose a product and use factory to create the object based on the user input
    while (myProduct == nullptr)
    {
        std::cout << "Chose one of the above products : ";
        std::cin >> choosedProduct;

        myProduct = ProductFactory.createProduct(choosedProduct);

    } ;


    // display all the attributes of the product
    myProduct->DisplayComponents();
    // let the user to add values to components
    myProduct->setAttributes();
    // display the product ith the values of the user
    first.buyProduct(myProduct);
    std::cout << "Do you want to continue: Y or N :" ;
    std::cin >> finish;
    }
    std::cout << first.getName() << " bought :" << std::endl;
    first.checkOut();
    system("pause");
}

\$\endgroup\$
  • \$\begingroup\$ Rather than post this answer, you might want to post a new question with a link to this question, especially if you have made significant changes. \$\endgroup\$ – pacmaninbw May 15 '19 at 17:18

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.