   Guess an object game response [c++]
  I've made a program, for a unit at university, where you are supposed to think of an object and the computer should try to guess the object. The program builds a tree on user input.I would love some response on the code, function names, Object Orientation and if something can be refractioned. Just on everything realy :)main.cpp:#include <string>#include <fstream>#include <iostream>#include "DecisionTree.h"#include "Treenode.h"using namespace std;bool openInFile(ifstream &inFile, string &fileName){bool ret = false;cout << endl << "Do you want to load a tree file?";if(DecisionTree::getYesNoResponse()){ret = true;cout << endl << "Enter input filename(same file will be used for saving.)" << endl;fileName = DecisionTree::getString();inFile.open(fileName.c_str(), ios::in);}return ret;}void openOutFile(ofstream &outFile, string fileName){if(fileName.empty()){cout << endl << "Enter filename to save tree in." << endl;fileName = DecisionTree::getString();}outFile.open(fileName.c_str());}bool playAgain(){cout << endl << "Do you want to play again?";return DecisionTree::getYesNoResponse();}int main(){DecisionTree questionTree;string fileName;ifstream inFile;if(openInFile(inFile,fileName))questionTree.loadTree(inFile);elsequestionTree.createInitialTree();inFile.close();do{questionTree.play();}while(playAgain());ofstream outFile;openOutFile(outFile, fileName);questionTree.saveTree(outFile);outFile.close();return 0;}treenode.h#ifndef TREENODE_H#define TREENODE_H#include <string>using namespace std;class Treenode{private:string m_questionOrAnswere;    Treenode* m_yes;    Treenode* m_no;public:    Treenode(Treenode* yesPtr, Treenode* noPtr,string q_aStr);Treenode();Treenode(const Treenode &rhs){copy(rhs);}~Treenode(){};Treenode & operator = (const Treenode & rhs);void copy(const Treenode &rhs);Treenode* getNoNode() const {return m_no;}void setNoNode(Treenode* ptr){m_no = ptr;}string getQa() const {return m_questionOrAnswere;}void setQa(const string str){m_questionOrAnswere = str;}Treenode* getYesNode() const {return m_yes;}void setYesNode(Treenode* ptr){m_yes = ptr;}};#endifdecisiontree.h#ifndef DECISIONTREE_H#define DECISIONTREE_H#include "treenode.h"#include <cstdlib>using namespace std;class DecisionTree{private:    Treenode* m_rootNode;public:    DecisionTree(){};DecisionTree(const Treenode &rhs){copy(rhs);}~DecisionTree(){};DecisionTree & operator = (const DecisionTree & rhs);void copy(const DecisionTree &rhs);    void createInitialTree();Treenode* getRoot() const {return m_rootNode;}void loadTree(istream & in);void loadTreeTrav(istream & in, Treenode *nodePtr);void play();void playTravTree(Treenode *nodePtr);void saveTree(ostream & os);void saveTreeTrav(ostream & os,Treenode *nodePtr);static string getString();static bool getYesNoResponse();static bool validInput(char &input);};#endiftreenode.cpp#include "Treenode.h"Treenode::Treenode(){m_yes = NULL;m_no = NULL;}Treenode::Treenode(Treenode *yesPtr, Treenode *noPtr,string q_aStr){m_questionOrAnswere = q_aStr;m_yes = yesPtr;m_no = noPtr;}Treenode & Treenode::operator = (const Treenode & rhs){    copy(rhs);    return *this;}void Treenode::copy(const Treenode &rhs){    m_yes = rhs.getYesNode();    m_no = rhs.getNoNode();m_questionOrAnswere = rhs.getQa();}decisiontree.cpp#include "DecisionTree.h"#include <iostream>#include <string>DecisionTree & DecisionTree::operator = (const DecisionTree & rhs){    copy(rhs);    return *this;}void DecisionTree::copy(const DecisionTree & rhs){    m_rootNode = rhs.getRoot();}void DecisionTree::createInitialTree(){Treenode* horse = new Treenode(NULL,NULL, "Horse");Treenode* bird = new Treenode(NULL,NULL, "Bird");m_rootNode = new Treenode(bird, horse, "Does it have two legs?");}void DecisionTree::loadTree(istream & in){m_rootNode = new Treenode(NULL,NULL, "");loadTreeTrav(in, m_rootNode);}void DecisionTree::loadTreeTrav(istream & in, Treenode *nodePtr){string line;getline(in, line);nodePtr->setQa(line.substr(0,line.size() - 1));if(line[line.size()-1] == 'Q'){Treenode *newNoNode = new Treenode(NULL, NULL, "");nodePtr->setNoNode(newNoNode);loadTreeTrav(in, newNoNode);Treenode *newYesNode = new Treenode(NULL, NULL, "");nodePtr->setYesNode(newYesNode);loadTreeTrav(in, newYesNode);}}void DecisionTree::play(){cout << endl << "You must think of a single object. Pleas answere the following questions about this object." << endl;playTravTree(m_rootNode);}void DecisionTree::playTravTree(Treenode *nodePtr){if(nodePtr->getYesNode() == NULL && nodePtr->getNoNode() == NULL){cout << endl << "I guess that your object is a(n) " << nodePtr->getQa() << ". (Y/N): ";if(getYesNoResponse())cout << endl <<"Notice the superior intellect of the computer!" << endl;else{cout <<  endl <<"What object were you thinking of?: ";Treenode *newObject = new Treenode(NULL, NULL, getString());Treenode *curentCopy = new Treenode(NULL,NULL, nodePtr->getQa());nodePtr->setYesNode(newObject);nodePtr->setNoNode(curentCopy);cout << endl << "Pleas specify a question that has a yes answere for your object and a no answere for my guess: " << endl;nodePtr->setQa(getString());}}else{cout << endl << "Question: " << nodePtr->getQa() << endl;if(getYesNoResponse())playTravTree(nodePtr->getYesNode());elseplayTravTree(nodePtr->getNoNode());}return;}void DecisionTree::saveTree(ostream & os){saveTreeTrav(os,m_rootNode);}void DecisionTree::saveTreeTrav(ostream & os,Treenode *nodePtr){os << nodePtr->getQa();if(nodePtr->getYesNode() != NULL && nodePtr->getNoNode() != NULL){os << "Q" << endl;saveTreeTrav(os, nodePtr->getNoNode());saveTreeTrav(os, nodePtr->getYesNode());} elseos << "A" << endl;}string DecisionTree::getString(){//using cin.getline to get outside ms vs6.0++ bugchar input[256];string retStr;do{cin.getline(input, 256);retStr = input;}while(retStr.empty());return retStr;}bool DecisionTree::getYesNoResponse(){char input;while(cin >> input && !validInput(input)){}return(input == 'y' || input == 'Y');}bool DecisionTree::validInput(char &input){bool ret;if(input == 'y' || input == 'Y' || input == 'n' || input == 'N'){ret = true;}else{cout << endl << "INCORRECT RESPONSE - Pleas type Y/N" << endl; ret = false;}return ret;}[Edited by - Nanook on May 18, 2009 5:56:32 PM]
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); }};#endifThe 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);};  Refactored/answer - yes :pThanks! I've spent some time working through things now..- We're not allowed to use <algorithm> so I have to write a propper copy function my self, using what you wrote for the copy constructur I've written this:void Treenode::copy(const Treenode &rhs){m_questionOrAnswer = rhs.getQa();m_yes = rhs.m_yes ? new Treenode(*(rhs.m_yes)) : NULL;try{m_no = rhs.m_no ? new Treenode(*(rhs.m_no)) : NULL;}catch(...){delete m_yes;throw;}}- Hadn't used initialization lists before.. read abit about it and implemeted it..- We are told to have this as minimal for all classes (Thats why I got all the get/set methods)*Set methods for each data member.*Get methods for each data member.*Overloaded assignment operator.*Copy constructor.- about DecesionTree I was told by my tutor to have only a play function so it is hidden from the user how the tree actualy works... (not passing in a treenode cause that will tell the user that it uses a tree) so yeah.. now I'm confused :p but your saying not to have the DecisionTree class at all and just have all the build and save and everything in main?   ;  And..using namespace std;I was used to not using this from reading accelerated c++ some years back, but my tutor said with most compilers these days it doesn't matter?   ;Quote:Original post by NanookAnd..using namespace std;I was used to not using this from reading accelerated c++ some years back, but my tutor said with most compilers these days it doesn't matter?Your tutor would be wrong.Importing more names from a namespace than necessary can cause a name clash.  For example, suppose you're writing a library that will be used by other developers, and one of your header files has the statement using namespace std;.  Then, all of the names in the standard namespace will be imported into the global namespace, and there can be quite a few of them, even if you only include one or two of the standard C++ headers.  In case the user's code has names that are identical to standard names, they'll start getting errors about redefinitions.  If the algorithm header gets included in your header file, it will cause a clash with the user's code if they have functions with names like sort or fill, the std::string class might cause a conflict with the user's own string class, and so on.This is why its better to explicitly prefix all of your names, like std::sort or std::string, to avoid any unforeseen name clashes.   ;Quote:Original post by Captain_ThunderQuote:Original post by NanookAnd..using namespace std;I was used to not using this from reading accelerated c++ some years back, but my tutor said with most compilers these days it doesn't matter?Your tutor would be wrong.Importing more names from a namespace than necessary can cause a name clash.  For example, suppose you're writing a library that will be used by other developers, and it has the statement using namespace std;.  Then, all of the names in the standard namespace will be imported into the global namespace, and there can be quite a few of them, even if you only include one or two of the standard C++ headers.  If the algorithm header gets included in your header file, it will cause a clash with the user's code if they have functions with names like sort or fill, the std::string class might cause a conflict with the user's own string class, and so on.This is why its better to explicitly prefix all of your names, like std::sort or std::string, to avoid any unforeseen name clashes.Fair enough.. I'll stop listening to my tutor then :) I might be remembering wrong, but I think he said most compilers import from namespace only what is used in the code?   ;Quote:Original post by NanookQuote:Original post by Captain_ThunderQuote:Original post by NanookAnd..using namespace std;I was used to not using this from reading accelerated c++ some years back, but my tutor said with most compilers these days it doesn't matter?Your tutor would be wrong.Importing more names from a namespace than necessary can cause a name clash.  For example, suppose you're writing a library that will be used by other developers, and it has the statement using namespace std;.  Then, all of the names in the standard namespace will be imported into the global namespace, and there can be quite a few of them, even if you only include one or two of the standard C++ headers.  If the algorithm header gets included in your header file, it will cause a clash with the user's code if they have functions with names like sort or fill, the std::string class might cause a conflict with the user's own string class, and so on.This is why its better to explicitly prefix all of your names, like std::sort or std::string, to avoid any unforeseen name clashes.Fair enough.. I'll stop listening to my tutor then :) I might be remembering wrong, but I think he said most compilers import from namespace only what is used in the code?From C++ Coding Standards by Alexandrescu and Sutter:"In all headers, and in all implementation files before the last #include, always explicitly namespace-qualify all names. In implementation files after all #includes, you can and should write namespace using declarations and directives liberally. This is the right way to reconcile code brevity with modularity."   ;Quote:Original post by NanookRefactored/answer - yes :pThanks! I've spent some time working through things now..- We're not allowed to use <algorithm> so I have to write a propper copy function my self, using what you wrote for the copy constructur I've written this:<algorithm> is not used in my code to implement the copy constructor. It is used to implement the assignment operator, using the copy constructor. You can still do this; std::swap is a very simple operation that you can write yourself (or just inline).// written yourselftemplate <typename T>void swap(T& a, T& b) {  T x = a;  b = x;  a = b;}// Or just inlined in the assignment operatorTreenode& operator=(Treenode rhs) {  std::string temp = rhs.m_questionOrAnswer;  // You can figure out the rest of the swap following the above  // Likewise for the two pointers}Quote:- We are told to have this as minimal for all classes (Thats why I got all the get/set methods)*Set methods for each data member.*Get methods for each data member.Your instructor is full of shit. Sorry. :) People write these automatically in the real world all the time, because they are following someone else's idea, and it is a bad idea motivated by a misunderstanding of OOP.Quote:- about DecesionTree I was told by my tutor to have only a play function so it is hidden from the user how the tree actualy works... (not passing in a treenode cause that will tell the user that it uses a tree)But the class with the play function is called DecisionTree! :) Yes, you can make a thin wrapper class that holds the root node as a member. It just doesn't accomplish very much... but maybe still enough to be useful. Give the DecisionTree (maybe you want to rename it) the load-from-file functionality, and have it delegate the decision-making functionality. Maybe it also holds a pointer to the "current" node, so you can call play() in a loop in calling code, instead of using recursion.Quote:so yeah.. now I'm confused :p but your saying not to have the DecisionTree class at all and just have all the build and save and everything in main?Not in main; in free (not part of a class) functions.   ;Quote:Original post by NanookFair enough.. I'll stop listening to my tutor then :) I might be remembering wrong, but I think he said most compilers import from namespace only what is used in the code?It's not about "what is imported" (i.e. what ends up in your executable); it's about "how things are imported" (i.e. how the compiler decides whether a name refers to something from the namespace or not). Here's an example. Say I have a header like:#ifndef EVIL_H#define EVIL_Husing namespace std;class Evil {};#endifAnd then another header like#ifndef MY_VECTOR_H#define MY_VECTOR_Hstruct vector {  double x, y, z;}; // you know, like the graphics programming concept.And then a source file like#include <vector>#include "evil.h"#include "my_vector.h"// ... lalalala...Oops; now I'm trying to redefine std::vector, suddenly. And even if I re-ordered the headers, there would still be problems with saying which vector I wanted. (Without the using-declaration, "vector" would be my vector, and "std::vector" would be the standard library one.) To say nothing of all the other names that are defined in the std::namespace that are likely candidates for name collision.   ;Quote:// written yourselftemplate <typename T>void swap(T& a, T& b) {  T x = a;  b = x;  a = b;}Who ever wrote this swapper is up for some pushups...   