HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Binary tree/knowledge base design

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
knowledgedesignbinarytreebase

Problem

Currently I have a binary tree template setup where my main is using it with strings to make a question/answer game. I'm using a knowledge base that works as an interface to the binary tree that main uses. My code seems to work fine inserting/rearranging nodes as necessary but I think my design is pretty bad.

Is it usually best to just put the Node and BinaryTree classes in the same file since neither is independently useful?

I'm not sure how to not use class friending (assuming this is bad design) unless I just do away with the knowledge base. Without knowledge base being able to see private node members, it wouldn't be able to move it's "current" node pointer around the tree through questions/guesses unless it passed its current pointer by reference to a binary tree function that moved it each time.

Also with this setup, the binary tree doesn't really do anything except provide a root node. I'm also unsure how to move some traversal, etc. functionality from kbase to btree without just making a bunch of kbase functions that call their btree counterparts.

Is there a way to use std::swap in each file also without polluting with the utility header? This design looks to be bad OO-wise.

Node.h:

```
#ifndef NODE_H
#define NODE_H
#include

template
class Node {
friend class KnowledgeBase;
public:
Node() {lChild = rChild = nullptr;}
Node(T& newData) {lChild = rChild = nullptr; data = newData;}
~Node();
Node(const Node&);
bool isLeaf() {return (lChild == nullptr) && (rChild == nullptr);}
void setData(T newData) {data = newData;}
void setRchild(Node* newRchild) {rChild = newRchild;}
void setLchild(Node* newLchild) {lChild = newLchild;}

Node& operator=(const Node);
private:
T data;
Node *lChild;
Node *rChild;
};

template
Node::~Node() {
delete lChild;
delete rChild;
lChild = nullptr;
rChild = nullptr;
}

template
Node::Node(const Node& other) {
data = other.data;
lChild = ne

Solution

First of all, I'd rewrite the constructors to use member initialization lists instead of assignment in the body of the ctor (where possible). For example, Node's ctor could become:

Node(T& data) : data(data), lChild(nullptr), rChild(nullptr) { }


Your destructor for Node currently does some pointless work:

template
Node::~Node() {
    delete lChild;
    delete rChild;
    lChild = nullptr;
    rChild = nullptr;
}


After the dtor runs, the object no longer exists, so setting its members to nullptr accomplishes nothing useful. This can be reduced to just:

template
Node::~Node() {
    delete lChild;
    delete rChild;
}


I'd got a little further than just putting Node and BinaryTree in the same file. I'd make Node a nested class inside the BinaryTree class. I'd also add a get to the Node class, so KnowledgeBase can use it instead of accessing Node's private data directly.

template
struct BinaryTree {
    struct Node {
        friend class KnowledgeBase;

        Node() : lChild(nullptr), rChild(nullptr) { }
        Node(T& data) : data(data), lChild(nullptr), rChild(nullptr) { }
        Node(const Node&);

        bool isLeaf() { return (lChild == nullptr) && (rChild == nullptr); }
        void setData(T newData) { data = newData; }
        void setRchild(Node* newRchild) { rChild = newRchild; }
        void setLchild(Node* newLchild) { lChild = newLchild; }
        T get() const { return data; }

        Node& operator=(const Node);

        ~Node() {
            delete lChild;
            delete rChild;
        }

    private:
        T data;
        Node *lChild;
        Node *rChild;
    };

    BinaryTree() {root = nullptr;}
    ~BinaryTree() {delete root; root = nullptr;}
    BinaryTree(const BinaryTree&);

    BinaryTree& operator=(const BinaryTree other);
    friend class KnowledgeBase;
private:
    Node *root;
};


I think at least for now I'll pass on reviewing KnowledgeBase -- I'm reasonably certain it's buggy. Specifically, at least part of the time, KnowledgeBase::current can contain a pointer to a node in KnowledgeBase::bTree. However, its destructor attempts to delete current;. This will result in a double-delete, since bTree is a member of KnowledgeBase, and will be destroyed automatically when the KnowledgeBase object is destroyed.

Code Snippets

Node(T& data) : data(data), lChild(nullptr), rChild(nullptr) { }
template<typename T>
Node<T>::~Node() {
    delete lChild;
    delete rChild;
    lChild = nullptr;
    rChild = nullptr;
}
template<typename T>
Node<T>::~Node() {
    delete lChild;
    delete rChild;
}
template<typename T>
struct BinaryTree {
    struct Node {
        friend class KnowledgeBase<T>;

        Node() : lChild(nullptr), rChild(nullptr) { }
        Node(T& data) : data(data), lChild(nullptr), rChild(nullptr) { }
        Node(const Node&);

        bool isLeaf() { return (lChild == nullptr) && (rChild == nullptr); }
        void setData(T newData) { data = newData; }
        void setRchild(Node* newRchild) { rChild = newRchild; }
        void setLchild(Node* newLchild) { lChild = newLchild; }
        T get() const { return data; }

        Node& operator=(const Node);

        ~Node() {
            delete lChild;
            delete rChild;
        }

    private:
        T data;
        Node *lChild;
        Node *rChild;
    };

    BinaryTree() {root = nullptr;}
    ~BinaryTree() {delete root; root = nullptr;}
    BinaryTree(const BinaryTree&);

    BinaryTree& operator=(const BinaryTree other);
    friend class KnowledgeBase<T>;
private:
    Node *root;
};

Context

StackExchange Code Review Q#43101, answer score: 4

Revisions (0)

No revisions yet.