Refactored, perhaps? :)
In treenode:
- You've implemented your destructor to do nothing, which is no good. You allocate memory, so you should be deallocating it too.
- Your copy operation performs a shallow copy, which is also no good. You want parent nodes to have clear ownership of their children, which requires that you copy the children, recursively. This is easy to do, however.
- You don't need the set and get functions. Give the "pick the appropriate child according to the answer" functionality to the treenode. To build the tree, we will use recursion and build children before parents, so that we can just use the constructor.
- Don't put 'using namespace std;' in a header. There is no way for the .cpp file that includes the header to un-use the namespace.
- There should not be an 'e' at the end of "answer" :)
- Be consistent about how you indent, and don't assume that a tab is any specific size.
- Pass strings by const reference.
- Use initialization lists to implement constructors.
The treenode class is simple enough that I'm going to go ahead and implement it all in the header, within the class body. Pay close attention; there are several key points of technique here.
#ifndef TREENODE_H#define TREENODE_H#include <algorithm> // for std::swap; keep reading...#include <string>class Treenode{private: string m_questionOrAnswer; Treenode* m_yes; Treenode* m_no;public: // Here we see the initialization list at work. Treenode(const std::string& q_aStr, Treenode* yesPtr, Treenode* noPtr): m_questionOrAnswer(q_aStr), m_yes(yesPtr), m_no(noPtr) {} // We should not really have a no-arg constructor for this class. But we can certainly have one that only takes a string, representing the "answer" nodes. Treenode(const std::string& q_aStr): m_questionOrAnswer(q_aStr), m_yes(0), m_no(0) {} // Instead of having a copy member function, it's usual to implement // the copy constructor directly. Notice how the deep copy is // performed. Also notice how exception safety needs to be taken care // of manually, because more than one resource allocation is performed. Treenode(const Treenode& rhs): m_questionOrAnswer(rhs.m_questionOrAnswer) { m_yes = rhs.m_yes ? new Treenode(*(rhs.m_yes)) : 0; // If that failed, then nothing was allocated. But if // m_no fails, then m_yes was allocated, and needs to be // deallocated. try { m_no = rhs.m_no ? new Treenode(*(rhs.m_no)) : 0; } catch (...) { delete m_yes; throw; } } // Notice that the destructor will automatically clean up the // entire tree recursively when it gets called for the root. ~Treenode() { delete m_yes; delete m_no; // also note that it's safe to call delete on // a NULL pointer; it will simply do nothing. }; // Here we use the copy-and-swap idiom to implement operator=, and // ensure exception safety. void swap(const Treenode& rhs) { std::swap(m_questionOrAnswer, rhs.m_questionOrAnswer); std::swap(m_yes, rhs.m_yes); std::swap(m_no, rhs.m_no); } // We implicitly make a copy using the copy constructor, by passing // the argument by value. Treenode& operator= (Treenode rhs) { // If the copy constructor failed, then (a) we don't get here, // and don't have to worry about it; (b) everything was cleaned // up because of how we wrote the copy constructor. // If it succeeded, we swap the node contents: swap(rhs); // Now the copy holds our old subtrees and label, and we have // the copy's subtrees and label (as we wanted). return *this; } // Finally, the destructor is called for the copy (because it's // stack-allocated), which cleans up our old subtrees and label. // Functionality to "trace" the tree. Treenode* getChild(boolean response) const { return response ? m_yes : m_no; } // We do need an accessor for the question/answer. const std::string& getText() const { return m_questionOrAnswer; } // And we need to check if we have an answer or a question. bool isLeaf() const { return !(m_yes || m_no); }};#endif
The DecisionTree class is not really very useful at all; it doesn't make good abstractions or separate responsibility very well. Instead, just have a free function that accepts a TreeNode (the root of a tree) and plays the game; a free function that accepts a stream and constructs a tree (returning the root node), etc.
As for your file format, put the question/answer marker at the beginning of the line; that way, you can read the marker, and then read the rest of the line.
void play(const Treenode* node) { std::cout << node->getText(); if (!node->isLeaf()) { play(node->getChild(getResponse()); }}Treenode* build(std::istream& input) { char c = input.get(); std::string text; std::getline(input, text); if (c == 'A') { return new Treenode(text); } // Otherwise, build children recursively first. Treenode* yes = build(input); Treenode* no = build(input); return new Treenode(text, yes, no);}
