0

I have two classes that are essentially string classes that look like this. The first class holds a string as a member and the second class also holds a string plus an array of pointers for the 'MenuItem' class and also a tracker.

const int MAX_NO_OF_ITEMS = 10;

    class Menu; // forward declaration

    class MenuItem {
        char* Menuitem;
        //member functions
        MenuItem(); 
        ...
        ~MenuItem();
        friend Menu; 
    };

    class Menu {
    private:
        char* m_Title;
        MenuItem* m_MenuItems[MAX_NO_OF_ITEMS];
        int m_menuTracker;
     ... //other functions not shown
    }

I want to be able to deep copy one Menu object to another Menu object but the way I am doing it seems do be as if it were a shallow copy when it comes to copying over the 'm_MenuItems'. I know for sure that the 'm_Title' member is getting deep copied as I am creating a 'new' char* for it but the problem arises when I am trying to copy the source 'm_MenuItems' to the destination as they end up sharing the same memory. This causes problems because when the deconstructor is called, it is called twice on the same object, causing my program to crash.

Here is my copy and assignment code:

    Menu& Menu::operator=(const Menu& src) {

        if (this != &src && src.m_Title != nullptr) {
            delete[] m_Title;
            m_Title = nullptr;
           
            m_Title = new char[strlen(src.m_Title) + 1];
            strcpy(m_Title, src.m_Title);
            
            //if current object has menuItems, then delete to make room for src. 
            for (int i = 0; i < m_menuTracker; i++) {
                delete m_MenuItems[i];
                this->m_MenuItems[i] = nullptr;
            }

This following for-loop is where my problem arises...

            if (src.m_MenuItems[0] != nullptr) {
                for (int i = 0; i < src.m_menuTracker; i++) {
                    m_MenuItems[i] = src.m_MenuItems[i];
                }
        
                this->m_menuTracker = src.m_menuTracker;
            }
            else
                this->m_menuTracker = 0;
        }
        else {
            this->setEmpty();
        }
        
        return *this;
    }

How am I supposed to allocate a new block of memory for the destination 'm_MenuItems'?

AKidNamedCoder
  • 3
  • 3
  • 1
    Always avoid `new` in C++. Always use `std::string` and `std::vector`. They're less buggy, and much faster, in most cases. If you used them, your deep copy would be `Menu& Menu::operator=(const Menu& src)=default;` – Mooing Duck Jul 09 '20 at 21:50
  • Even if you were to try and tackle this problem without using `std::string` or `std::vector`, you should start with `MenuItem` and making sure that has correct copy semantics, not `Menu`. – PaulMcKenzie Jul 09 '20 at 22:05
  • That’s where the tricky part comes into play. This project specifically to prohibit copies or assignments in the MenuItem class. Forgot to mention that lol – AKidNamedCoder Jul 09 '20 at 22:07
  • Then you should write the code to prevent copies. Right now, given the code you've shown, anyone can make copies of `MenuItem`. You should be making the copy constructor and assignment operator `delete`-ed, or declare them but don't implement them. This should have been covered in the C++ course you're taking if this is a requirement. In addition, why are you not using `std::string`, at the very least? – PaulMcKenzie Jul 09 '20 at 22:08
  • Also, you really should be implementing the copy constructor and destructor first, and leave the assignment operator for later. The reason why is that if you have a working copy constructor and destructor, the assignment operator takes a minute to get working by using simple `swap` calls. – PaulMcKenzie Jul 09 '20 at 22:12
  • [More on Paul's comment about using swaps in the assignment operator](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). – user4581301 Jul 09 '20 at 22:23

0 Answers0