0

I have the following header class:

class Tree
{
private:
    string label;
    vector<Tree*> children;
    void free(Tree* tree);
    Tree* copyFrom(const Tree* other);

public:
    Tree(string _label, vector<Tree*> _children= vector<Tree*>());
    Tree(const Tree& other);
    void addChild(Tree* child);
    Tree& operator=(const Tree& other);
    Tree* addChild(Tree* parent, string label);
    void removeChild(vector<Tree*>::iterator position);
    void insertTree(Tree* tree, vector<Tree*>::iterator position);
    ~Tree();
};

and following implementation

#include "Tree.h"

string Tree::getLabel() const
{
    return label;
}

Tree* Tree::copyFrom(const Tree* other)
{
    if (other == nullptr)
    {
        return nullptr;
    }
    Tree* result = new Tree(other->label);
    vector<Tree*> _children;
    for (int i = 0; i < other->children.size(); i++)
    {
        _children.push_back(copyFrom(other->children[i]));
    }
    result->children= _children;
    
    return result;
}

void Tree::free(Tree* tree)
{
    if (this == nullptr)
    {
        return;
    }
    for (int i = 0; i < tree->children.size(); i++)
    {
        delete tree->_children[i];
    }
    tree->children.clear();
    
}

Tree::Tree(string _label, vector<Tree*> _children)
    :label(_label), children(_children)
{

}

Tree::Tree(const Tree& other)
{
    const Tree* pointer = &other;
    copyFrom(pointer);
    this->label = pointer->label;
    this->children= pointer->children;
    pointer = nullptr;
}




Tree& Tree::operator=(const Tree& other)
{
    if (this != &other)
    {
        free(this);
        const Tree* pointer = &other;
        copyFrom(pointer);
        this->label = pointer->label;
        this->children= pointer->children;
        pointer = nullptr;
    }
    return *this;
}



Tree::~Tree()
{
    free(this);
}

void Tree::addChild(Tree* child)
{
    children.push_back(child);
}

Tree* Tree::addChild(Tree* parent, string label)
{
    if (parent == nullptr)
    {
        throw "No parent";
    }
    Tree* result = new Tree(label);
    parent->children.push_back(result);
    return result;
}

void Tree::removeChild(vector<Tree*>::iterator position)
{
    if (children.size() == 0)
    {
        throw "No children to delete from!";
    }
    int index = position - children.begin();
    Tree* toDelete = children.at(index);
    children.erase(position);
    delete toDelete;
}

void  Tree::insertChild(Tree* tree, vector<Tree*>::iterator position)
{
    if (tree== nullptr)
    {
        throw "Tree cannot be null!";
    }
    children.insert(position, tree);
}

I have some struggles with my destructor, copy constructor and consequently, operator=. I believe my destructor works correctly, but my copy constructor is giving me a hard time, because it is supposed to create a deep copy, but when I copied an object, let's call the copy copyTree and the original firstTree, and then removed a child from firstTree, the change affected copyTree as well. What am I doing wrong?

ManGoose
  • 25
  • 5
  • 1
    All these struggles are because of a very common C++ problem called "Pointless Use Of Pointers". All these struggles go away if `children` becomes just a `vector`. Poof! No more destructor, copy constructor, and the `=` operator. C++ will write them for you! – Sam Varshavchik Jan 08 '23 at 20:49
  • But how can I make children of type vector ? I believe an object cannot contain itself or a collection of itself, because then the compiler cannot figure how much memory the class requires, or am I wrong? – ManGoose Jan 08 '23 at 20:51
  • 1
    What makes you think that the object would contain itself? A `std::vector`, itself takes up, usually, just a few bytes, only enough for a single pointer and a couple of counters. Every C++ compiler will have no problems computing the class's entire size. – Sam Varshavchik Jan 08 '23 at 20:53
  • I don't know, during our lectures all our professors implemented binary tree with pointer to left and right and they told us an object cannot contain object of the same type but it can contain a pointer to one, so I assumed I had to do same. I guess I could substitue it with vector of Tree instead of Tree*, but even so, I still wonder why my code does not work as I expected. Thank you for the workaround :) – ManGoose Jan 08 '23 at 20:56
  • 1
    You might be surprised to learn that the `std::vector`'s only contents are a pointer. A measly pointer to the values in the vector. So, you see, that's in fact all that this `Tree` object ends up containing: a pointer to a bunch of vectors. It doesn't contain "itself" in any form or fashion. Although your professors are right, an object cannot contain itself, but that's not what a `std::vector` is, and your professors apparently don't understand how a `std::vector` works. You won't be the first one who posted evidence, on Stackoverflow, of incompetent C++ instructors. – Sam Varshavchik Jan 08 '23 at 21:01
  • 1
    @Tuna239 W.r.t. pointers, please show [this](https://youtu.be/YnWhqhNdYyk) to your professor. (If possible, figure out if you can attend another professor’s classes.) As for your question on why the copy-constructor doesn’t work: You first make all the recursive effort in `copyFrom()` — yet then you **undo** all of it and let all the copied memory **leak** on this line: `this->children= pointer->children;` ← That there is a “partially shallow” copy that replaces the deep copy above. Anyway, as @SamVarshavchik said, you should avoid avoidable pointers. Also, check it with `valgrind`; it helps. – Andrej Podzimek Jan 08 '23 at 21:14
  • Thank you everybody, It might be that I've misunderstood my professors or maybe they enforce us to use pointers in order to learn how they work. I will look into valgrind, I didn't know there was such a tool. Unfortunately, I don't think that me showing them the video would change anything. All our university courses are based on C++, so even if an algorithm is correct, memory leaks result in the lowest grade. – ManGoose Jan 08 '23 at 21:26

1 Answers1

0

The replies provided me with great workarounds for this hassle, but to answer my question, the concrete problem was that I forgot to assign the result from copyFrom, that is:

Box::Box(const Box& other)
{
    const Box* pointer = &other;
    Box* result = copyFrom(pointer);
    this->label = result->label;
    this->boxes = result->boxes;
    this->souvenirs = result->souvenirs;
    pointer = nullptr;
}

and same for operator =

ManGoose
  • 25
  • 5