patterncppMinor
Binary tree/knowledge base design
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
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
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
Is there a way to use
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
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:
Your destructor for
After the dtor runs, the object no longer exists, so setting its members to
I'd got a little further than just putting
I think at least for now I'll pass on reviewing
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.