Reducing duplicate code for function implementation

Question

Unfortunately, I get stuck with a problem with duplicated source code. Here is a small example to illustrate my problem:

class cPlayer
{
public:

    struct Properties
    {
        std::vector<cStreet*>    Streets;
        std::vector<cHouse*>     Houses;
        std::vector<cComputer*>  Computers;
        std::vector<cBook*>      Book;
    };

    cPlayer(std::string name) : m_name{name}{};
    ~cPlayer(){};
    std::string         m_name{};
    Properties          m_Properties;
    
    // function overloaded
    void buy(cStreet& Street);
    void buy(cHouse& House);
    void buy(cComputer& Computer);
    void buy(cBook& Book);
};

void cPlayer::buy(cStreet& Street)
{
    std::cout << m_name.c_str() << " : Do you want buy this Street?" << std::endl;
    //Todo: Decision (here yes)
    m_Properties.Streets.push_back(&Street);
};

void cPlayer::buy(cHouse& House)
{
    std::cout << m_name.c_str() << " : Do you want buy this House?" << std::endl;
    //Todo: Decision (here yes)
    m_Properties.Houses.push_back(&House);
};

void cPlayer::buy(cComputer& PC)
{
    std::cout << m_name.c_str() << " : Do you want buy this PC?" << std::endl;
    //Todo: Decision (here yes)
    m_Properties.Computers.push_back(&PC);
};

void cPlayer::buy(cBook& Book)
{
    std::cout << m_name.c_str() << " : Do you want buy this Book?" << std::endl;
    //Todo: Decision (here yes)
    m_Properties.Book.push_back(&Book);
};

So the 4 member functions buy() actually all have the same logic. However, an individual text is output and the individual std::vector is always used. It would of course be much more elegant to implement the function only once. But how? I had only thought of templates, but how can I switch the correct vector() to save the property?

Question after question. Would be great if I could get food for thought, as such a "problem" often appears in my source code.

THANK YOU!

You can make the method a template with two parameters, (1) the type it works with and (2) a member pointer to the corresponding member of Properties. — Andrej Podzimek, Dec 16 '21 at 11:47
Note: [**don't** define a destructor if not necessary (also called the rule of zero](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-zero). — JHBonarius, Dec 16 '21 at 13:36

JHBonarius · Answer 1 · 2021-12-16T12:18:37.720

1

** A ** solution here is to use inheritance. e.g.

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

class Property
{
public:
    virtual char const* Name() const = 0;
};

class Street : public Property
{
private:
    static constexpr auto name {"street"};
public:
    char const* Name() const override
    {
        return name;
    }
};


//... etc

class Player
{
    std::vector<Property const*> properties;
public:
    void buy(Property const& property) {
        std::cout << /*...*/ " : Do you want to buy this "
            << property.Name()
            << "?\n";
        // if (yes)
        properties.push_back(&property);
    }
};

int main() {
    Player me {};
    const Street street {};

    me.buy(street);
}

edited Dec 16 '21 at 12:18

answered Dec 16 '21 at 12:05

JHBonarius

10,824
3
22
41

Thanks for the hint / the idea. But that means that I manage all properties in a single std::vector<> right? In other words, if I just want to sell the books later (maybe by a sell() function), I first have to look for all the objects of the book type in this std::vector<> right? Is there actually a way to do this in C++ without even saving the type for the class cBook, cHouse, etc., for example in the form of an enum? – ThomasAlvaEdison Dec 17 '21 at 10:15
@ThomasAlvaEdison that's why I say that it's **A** solution. there are many. It all depends on the application. By the way, the solution you accepted is overly complicated. Complicated code is harder to maintain and easy to make mistakes. Often simpler is better. – JHBonarius Dec 17 '21 at 11:53

Jason · Accepted Answer · 2022-02-24T17:56:56.077

One possible way of doing this is using templates and std::map. In particular, by making the buy member function to be a member function template as shown below:

Method 1

Step 1

Creating a std::map

    std::map<std::type_index, void*> myMap{{typeid(cStreet*), &m_Properties.Streets}, 
                                           {typeid(cHouse*), &m_Properties.Houses}, 
                                           {typeid(cComputer*), &m_Properties.Computers}, 
                                           {typeid(cBook*), &m_Properties.Book}};

Step 2

Add declaration for member function template buy<> inside class cPlayer

template<typename T> void buy(T& Arg);

Step 3

Implement buy<>

template<typename T> void cPlayer::buy(T& Arg) // - STEP 3
{
    std::cout << m_name.c_str() << " : Do you want buy this ?" <<typeid(Arg).name() << std::endl;
    //Todo: Decision (here yes)
    (*static_cast<std::vector<decltype(&Arg)>*>(myMap.at(typeid(&Arg)))).push_back(&Arg);
}

Overall Modification

So the modified code will look something like below:

class cPlayer
{
public:

    struct Properties
    {
        std::vector<cStreet*>    Streets;
        std::vector<cHouse*>     Houses;
        std::vector<cComputer*>  Computers;
        std::vector<cBook*>      Book;
    };
    
    cPlayer(std::string name) : m_name{name}{};
    ~cPlayer(){};
    std::string         m_name{};
    Properties          m_Properties;
    
    //create std::map -                             STEP 1
    std::map<std::type_index, void*> myMap{{typeid(cStreet*), &m_Properties.Streets}, 
                                           {typeid(cHouse*), &m_Properties.Houses}, 
                                           {typeid(cComputer*), &m_Properties.Computers}, 
                                           {typeid(cBook*), &m_Properties.Book}};
    
    //create member function template -             STEP 2
    template<typename T> void buy(T& Arg);
    
};
template<typename T> void cPlayer::buy(T& Arg) // - STEP 3
{
    std::cout << m_name.c_str() << " : Do you want buy this ?" <<typeid(Arg).name() << std::endl;
    //Todo: Decision (here yes)
    (*static_cast<std::vector<decltype(&Arg)>*>(myMap.at(typeid(&Arg)))).push_back(&Arg);
}

Method 2

This uses std::any as the mapped_type of the std::map.

class cPlayer
{
public:

    struct Properties
    {
        std::vector<cStreet*>    Streets;
        std::vector<cHouse*>     Houses;
        std::vector<cComputer*>  Computers;
        std::vector<cBook*>      Book;
    };
    
    cPlayer(std::string name) : m_name{name}{};
    ~cPlayer(){};
    std::string         m_name{};
    Properties          m_Properties;
    
   //create std::map -                             STEP 1 
   std::map<std::type_index, std::any> myMap{ {typeid(cStreet*), std::ref(m_Properties.Streets)}, 
                                              {typeid(cHouse*), std::ref(m_Properties.Houses)}, 
                                              {typeid(cComputer*), std::ref(m_Properties.Computers)}, 
                                              {typeid(cBook*), std::ref(m_Properties.Book)}
};   
    //create member function template -             STEP 2
    template<typename T> void buy(T& Arg);
    
};

    
template<typename T> void cPlayer::buy(T& Arg) {// - STEP 3
  
     std::cout << m_name.c_str() << " : Do you want buy this ?" <<typeid(Arg).name() << std::endl;
     std::any_cast<std::reference_wrapper<std::vector<T*>>>(myMap.at(typeid(T*))).get().push_back(&Arg);     
}

score 0 · Answer 3 · answered Dec 16 '21 at 12:08

It is really not code duplication, as the code is slightly different for each type of property. It is similar, but not identical.

You could move some of the code to a separate helper function and get the buy code down to

void cPlayer::buy(cStreet& Street)
{
    if (ConfirmPurchase("Street"))
       m_Properties.Streets.push_back(&Street);
};

But that's about it, IMO. Don't make the code extra complicated just to reduce duplication.

score -1 · Answer 4 · answered Dec 16 '21 at 11:49

If you like you can refactor it to

void cPlayer::buy(cStreet& Street)
{
     buy_impl(Street,m_Properties.Streets,"some text");
}

and similar for the others, with

template <typename T,typename U>
void buy_impl(const T& t, U& prop,const std::string& message) {
    std::cout << message;
    U.push_back(&t);
}

Though, not sure if added complexity is worth the little savings. Your methods already contain not much more than what is different between them.