So i wanted to write a Hangman game and i started it, as im newbie programmer it took me several days to track all the bugs etc. Meanwhile i want to get good habits on designing and i read that Getters and Setters arent the best way to go for a class design. I tryed my best to lead this project to a good design but i need a feedback to see if im on the right track, please, comment on everything you see that are lame or that can be build better.
Im scared of comments like "looks good, decent design" etc. please be strict i really worked hard on it :D

.cpp

#include "HANGMAN.h"

//Ctor and Dtor
HANGMAN::HANGMAN(){


    SetConsoleTitle ("Hangman v1.0");
    hStdout = GetStdHandle (STD_OUTPUT_HANDLE);


    exitGame = false;
    inGame = false;


    lenofFile = CountLinesOfFile ("words.txt"); //Count lines
    CreateWordsBuf (lenofFile, "words.txt");    //Allocate and store them


    srand (time(NULL)); //seed for picking element index from _wordsBuf


    CreditsIntro();


}
HANGMAN::~HANGMAN(){
    //deallocate
    delete[] _wordsBuf;
    _wordsBuf = NULL;


    delete[] _hiddenWord;
    _hiddenWord = NULL;


    delete[] _letterBuf;
    _letterBuf = NULL;
}




void HANGMAN::Set_exitGame (bool b){ //Flag for the main loop
    exitGame = b;
}
bool HANGMAN::Get_exitGame(){
    return exitGame;
}
void HANGMAN::Set_inGame(bool b){    //Flag for inside game loop
    inGame = b;
}
bool HANGMAN::Get_inGame (){
    return inGame;
}




void HANGMAN::ClearScreen(){  //Clears the screen in radius {80,30}
    COORD homeCoords = {0,0};


    //{80,30} is enough to clear the previous game
    SetConsoleCursorPosition (hStdout, homeCoords);
    for (int row = 0; row < 30; row++){
        for (int col = 0; col < 80; col++){
            cout << ' ';
        }
    }
    SetConsoleCursorPosition (hStdout, homeCoords);
}
void HANGMAN::CreditsIntro(){ //Badass intro and tips
    COORD dotsCoords = {0,2};
    int tip = rand() % 2;


    cout << "GAME BY: COFFECAT" << endl << endl;


    //pick a tip
    cout << endl << endl << "   Tip of the day:" << endl;
    switch (tip){
        case 0:
            cout << "Separate the word's in the text file with a whitespace.";
            break;
        case 1:
            cout << "Don't let your user see the word's from the text file.";
            break;
    }


    //output 3 dots with short delay, clear and output again
    SetConsoleCursorPosition (hStdout, dotsCoords);
    for (int i = 0; i < 2; i++){
        cout << "."; Sleep (500);
        cout << "."; Sleep (500);
        cout << "."; Sleep (500);
        cout << '\r' << "   ";
        cout << '\r';  Sleep(500);
    }


    ClearScreen();
}




int HANGMAN::CountLinesOfFile (const char* fname){ //Counts number of words in .txt file


    int n = 0;
    string word;


    ifstream file (fname);


    //read all lines form the file and count them
    if (file.is_open()){


        while (!file.eof()){
            getline (file, word, ' ');
            n++;
        }
        file.close();
    }
    else{
        cout << "Failed to open file for CountLinesOfFile (const char*)" << endl;
    }


    return n;
}
void HANGMAN::CreateWordsBuf (int len, const char* fname){ //Allocate _wordsBuf and store words .txt
    string line;
    int index = 0;
    _wordsBuf = new string[len];


    ifstream file (fname);


    //read words and store them in own element
    if (file.is_open()){


        while (!file.eof()){


            getline (file, line, ' ');
            _wordsBuf[index++] = line;
        }
        file.close();
    }
    else{
        cout << "Failed to open words.txt for CreateWordBuf (int, const char*)" << endl;
    }
}




void HANGMAN::Pull_randWord (int filelen){ //Pulls a word from _wordsBuf
    int randVal;
    randVal = rand() % filelen;


    randWord = _wordsBuf[randVal];
}
void HANGMAN::CreateLetterBuf(int len){    //Allocate _letterBuf and fills with whitespaces


    //allocate array and fill it whitespaces (inputed letters will be pushed)
    _letterBuf = new char[len];
    for (int i = 0; i < len; i++){
        _letterBuf[i] = ' ';
    }
}
void HANGMAN::CreateHiddenWord(int len){   //Allocate _hiddenWord buf and fills with underscores


    //allocate array and fill it with underscores for hidden word UI
    _hiddenWord = new char[len];
    for (int i = 0; i < len; i++){
        _hiddenWord[i] = '_';
    }
}
void HANGMAN::OutputHiddenWord(){ //Outputs _hiddenWord buf


    //outputs the underscores and adds a whitespace between each for better reading
    for (int x = 0; x < lenofRW; x++){
        cout << _hiddenWord[x];
        if (x != lenofRW - 1){
            cout << " ";
        }
    }
}
void HANGMAN::OutputIngameUI(){   //Requires _hiddenWord buf to be allocated and output simple UI


    cout << "Tries left: " << triesLeft;
    cout << endl << endl;


    OutputHiddenWord();
    cout << endl << endl;


    cout << "Letter input: ";
}
void HANGMAN::UpdateHiddenWord(){ //Set cursor at proper pos and re-writes _hiddenWord buf
    COORD hiddenWordCoords = {0,2};


    SetConsoleCursorPosition (hStdout, hiddenWordCoords);
    OutputHiddenWord();
}
void HANGMAN::UpdateTriesLeft(){  //Set cursor at proper pos and re-write triesLeft value
    COORD triesLeftCoords = {0,0};
    COORD eraseDigit = {13,0};


    //put whitespace at the second digit when triesLeft goes under 9
    if (triesLeft < 10){
        SetConsoleCursorPosition (hStdout, eraseDigit);
        cout << ' ';
    }
    SetConsoleCursorPosition (hStdout, triesLeftCoords);
    cout << "Tries left: " << triesLeft;
    cout << endl << endl;
}




void HANGMAN::CreateNewGame(){   //Pulls new randWord, alloc bufs, value for triesLeft/lenofLetterB


    ClearScreen();


    //pull new randWord and create _hiddenWord buf
    Pull_randWord (lenofFile);
    lenofRW = randWord.size();
    CreateHiddenWord (lenofRW);


    //set value for triesLeft and create _letterBuf buf
    triesLeft = lenofRW;
    lenofLetterB = triesLeft * 2; //hold correct and incorect letters
    CreateLetterBuf (lenofLetterB);


    inGame = true;
}
void HANGMAN::CheckForEndGame(){ //Compares randWord letters with _hidenWord buf, checks triesLeft


    //check if triesLeft are wasted, if so, end the current game
    if (triesLeft == 0){
        cout << endl << endl << endl << endl << "WRONG!";
        Sleep (2000);
        inGame = false;
    }


    //compare _hiddenWord and randWord if all letters match
    int count = 0;
    for (int j = 0; j < lenofRW; j++){
        if (_hiddenWord[j] == randWord[j]){
            count++;
        }
    }
    //if _hiddenWord match randWord, end the current game
    if (count == lenofRW){
        cout << endl << endl << endl << endl << "CORRECT!";
        Sleep (2000);
        inGame = false;
    }
}




void HANGMAN::Set_input(char val){ //Used for reset
    input = val;
}
char HANGMAN::Get_input(){
    return input;
}
void HANGMAN::AskInputGetch (){ //Works with getch()
    input = getch();
}
void HANGMAN::AskLetterInput(){ //Ask for single letter, can delete it before confirming with Enter
    COORD letterInpCoords; //X and Y values will be changed trough the function loops
    COORD letterInpOrigCoords = {14,4}; //permanent values for X and Y
    bool letterConfirmed = false;


    //SetConsoleCursorPosition (hStdout, letterInpOrigCoords); //set cursor at begining


    //loop until a letter is pressed and then ENTER key
    while (!letterConfirmed){


        //if letter is deleted, reset coords so can re-write the buffer
        letterInpCoords.X = 14;
        letterInpCoords.Y = 4;


        //search free element spot for input
        for (int i = 0; i < lenofLetterB; i++){
            if (_letterBuf[i] == ' '){  //if finds a whitespace element,
                letterInpCoords.X += i; //take value of i + 14 for X coordinates
                SetConsoleCursorPosition (hStdout, letterInpCoords); //set proper pos for input


                //loop until a letter only is pressed
                do {
                    inpLetter = getch();
                } while ((inpLetter != 'A' && inpLetter != 'a') &&
                         (inpLetter != 'B' && inpLetter != 'b') &&
                         (inpLetter != 'C' && inpLetter != 'c') &&
                         (inpLetter != 'D' && inpLetter != 'd') &&
                         (inpLetter != 'E' && inpLetter != 'e') &&
                         (inpLetter != 'F' && inpLetter != 'f') &&
                         (inpLetter != 'G' && inpLetter != 'g') &&
                         (inpLetter != 'H' && inpLetter != 'h') &&
                         (inpLetter != 'I' && inpLetter != 'i') &&
                         (inpLetter != 'J' && inpLetter != 'j') &&
                         (inpLetter != 'K' && inpLetter != 'k') &&
                         (inpLetter != 'L' && inpLetter != 'l') &&
                         (inpLetter != 'M' && inpLetter != 'm') &&
                         (inpLetter != 'N' && inpLetter != 'n') &&
                         (inpLetter != 'O' && inpLetter != 'o') &&
                         (inpLetter != 'P' && inpLetter != 'p') &&
                         (inpLetter != 'Q' && inpLetter != 'q') &&
                         (inpLetter != 'R' && inpLetter != 'r') &&
                         (inpLetter != 'S' && inpLetter != 's') &&
                         (inpLetter != 'T' && inpLetter != 't') &&
                         (inpLetter != 'U' && inpLetter != 'u') &&
                         (inpLetter != 'V' && inpLetter != 'v') &&
                         (inpLetter != 'W' && inpLetter != 'w') &&
                         (inpLetter != 'X' && inpLetter != 'x') &&
                         (inpLetter != 'Y' && inpLetter != 'y') &&
                         (inpLetter != 'Z' && inpLetter != 'z'));


                break; //break loop if letter is inputed
            }
            //if any time comma's are used or other letters, count them
            else
                letterInpCoords.X += 1;
        }




        //output inputed letter on the same coords that was asked to
        cout << inpLetter;


        //loop until ENTER or BACKSPACE key is pressed
        input = -1; //reset
        while (input != 13 && input != 8){


            input = getch();
            switch (input){
                //ENTER KEY -- confirms the letter
                case 13:
                    /* save inputed letter in first whitespace element and add coma after it.
                    Confirm the letter flag to true and exit the function*/
                    for (int i = 0; i < lenofLetterB; i++){
                        if (_letterBuf[i] == ' '){
                            _letterBuf[i] = inpLetter;
                            cout << ",";
                            letterConfirmed = true;
                            break;
                        }
                    }
                    break; //case ENTER: ends




                //BACKSPACE KEY -- re-printing letters/visualy deleting
                case 8:
                    /* if still no letters in buffer, set cursor to begining and output whitespace,
                    then back one char so input can be be on the same spot */
                    if (_letterBuf[0] == ' '){
                        SetConsoleCursorPosition (hStdout, letterInpOrigCoords);
                        cout << ' ' << '\b';
                    }
                    /* if have inputed letter, set cursor to begining and re-write all letter's
                    and comma's */
                    else if (_letterBuf[0] != ' '){
                        SetConsoleCursorPosition (hStdout, letterInpOrigCoords);
                        for (int i = 0; i < lenofLetterB; i++){
                            if (_letterBuf[i] != ' '){        //if have letter in the element
                                cout << _letterBuf[i] << ","; //output it including comma after it
                            }
                            else {            //if no letter in the element
                                cout << ' ' << '\b'; //output whitespace to simulate deletion
                                break;        //then back one char so input be on the same spot
                            }
                        }
                    }


                    break; //case BACKSPACE: ends


            } //switch (input) ends


        } //while (input != ENTER && != BACKSPACE) ends


    } //while (!letterConfirmed) ends


}
void HANGMAN::CheckInputLetter(){ //Check if letter exist in randWord
    bool matchLetter = false;


    //compares inpLetter with randWord letters
    for (int x = 0; x < lenofLetterB; x++){
        if (inpLetter == randWord[x]){
            _hiddenWord[x] = inpLetter;
            matchLetter = true;
        }
    }
    //if inpLetter dont exist in randWord, decrease triesLeft
    if (!matchLetter){
        --triesLeft;
    }
}

.h

#ifndef HANGMAN_H#define HANGMAN_H
#include <iostream>
#include <fstream>
#include <conio.h>
#include <windows.h>
#include <string>
#include <time.h>


using namespace std;


class HANGMAN
{
    public:
        HANGMAN(); //Ctor and Dtor
        virtual ~HANGMAN();


        void Set_exitGame (bool b); //Flag for the main loop
        bool Get_exitGame();
        void Set_inGame (bool b);   //Flag for inside game loop
        bool Get_inGame();


        void ClearScreen();  //Clears the screen in radius {80,30}
        void CreditsIntro(); //Badass intro and tips


        int CountLinesOfFile (const char* fname); //Count lines of .txt file
        void CreateWordsBuf (int len, const char* fname);  //Allocates _wordsBuf and store words.txt


        void Pull_randWord (int filelen); //Pulls a word from _wordsBuf
        void CreateLetterBuf(int len);    //Allocate _letterBuf and fills with whitespaces
        void CreateHiddenWord (int len);  //Allocate _hiddenWord buf and fills with underscores
        void OutputHiddenWord(); //Outputs _hiddenWord buf
        void OutputIngameUI();   //Requires _hiddenWord buf to be allocated and output simple UI
        void UpdateHiddenWord(); //Set cursor at proper pos and re-writes _hiddenWord buf
        void UpdateTriesLeft();  //Set cursor at proper pos and re-write triesLeft value


        void CreateNewGame();  //Pulls new randWord, alloc bufs, value for triesLeft/lenofLetterB
        void CheckForEndGame(); //Compares randWord letters with _hidenWord buf, checks triesLeft


        void Set_input (char v); //Used for reset
        char Get_input();
        void AskInputGetch();   //Works with getch()
        void AskLetterInput();  //Ask for single letter, can delete it before confirming with Enter
        void CheckInputLetter(); //Check if letter exist in randWord




    private:
        //windows predef data types
        HANDLE hStdout; //std output handle


        //game
        int triesLeft;
        bool exitGame; //flag for the main loop
        bool inGame;   //flag for inside game loop


        //input
        char input;     //UI input
        char inpLetter; //input only for letters
        char* _letterBuf;
        int lenofLetterB; //lenth of _letterBuf


        //file
        int lenofFile; //lenth of .txt file
        string* _wordsBuf;


        //word
        char* _hiddenWord;
        string randWord; //Pull_randWord returns this variable
        int lenofRW;     //lenth of randWord
};


#endif // HANGMAN_H

main.cpp

#include "HANGMAN.h"

using namespace std;


int main()
{
    HANGMAN HANGM;


    while (!HANGM.Get_exitGame()){


        HANGM.ClearScreen();
        cout << "HANGMAN MENU:" << endl;
        cout << "------------" << endl;
        cout << "New Game -- Enter Key" << endl;
        cout << "Exit Game -- Esc Key" << endl << endl;


        //Loop until in MENU
        HANGM.Set_input (-1); //reset input
        while (HANGM.Get_input() != 13 && HANGM.Get_input() != 27){


            //Ask for user input
            HANGM.AskInputGetch();
            switch (HANGM.Get_input()){


                //New Game -- Enter Key
                case 13:
                    HANGM.CreateNewGame();
                    HANGM.OutputIngameUI();
                    break;


                //Exit Game -- Esc Key
                case 27:
                    HANGM.Set_exitGame (true);
                    break;


                default:
                    break;


            } //switch (input) ends


        } //while (ENTER && ESC) ends


        //Start Game loop
        while (HANGM.Get_inGame()){


            HANGM.AskLetterInput();
            HANGM.CheckInputLetter();


            HANGM.UpdateTriesLeft();
            HANGM.UpdateHiddenWord();


            HANGM.CheckForEndGame();


        } //while (inGame) ends


    } //while (!exitGame) ends




    return 0;
}

Feedback on design, comments, names, everything that you see that can be done in a better understanding way. How would you work with the function prototypes if you didnt had access to .cpp file? etc, please on everything

Recommended Answers

All 10 Replies

On a casual scan, I see that the section where you test for the input value being a letter can be simplified by using the isalpha() function (actually, it's usually implemented as a macro, but that's besides the point).

while (isalpha(inpLetter));

On a related note, the toupper() and tolower() functions can be used to simplify any case where you are accepting either an upper or lower case value as the same.
I'll look over it a little more carefully to see if I can suggest anything else.

I'm not sure what to say here, to be honest. While I don't see anything outright badly done, I don't feel very comfortable about some aspects of it, but I'm having some trouble pinning down just why. The way you mix UI and game logic elements is very uncomfortable to me, but not really wrong per se; I'm not certain how to advise you on improving it. The whole things seems vaguely off, as if there were some underlying misuse of the class system going on that is nonetheless entirely acceptable from a practical standpoint.

I wish I could be more specific about this, but there's just something vaguely inaesthetic about it to me.

Hey thanks, its my begining of my designing and i really wanna improve it. Very, very thanks for the feedback, really do need it for my feature programs.

On other hand, can i ask what you think abou the comments and the names in the whole program? Do you understand what the function does just by reading comments and what can i improve about them?
If you dont have acces to the .cpp file but only to .h, can you build a hangman game only with the prototypes (are they well made?) Thanks!

Here are some initial comments:

Style
Overall the coding style is OK, but a bit unusual. Naming functions like Pull_randWord is a bit unusual, but more importantly, it is not consistent with the rest of the code, you should pick one style and make it consistent. Then, using all upper-case letters for class names is a big no no. If there is one convention that you should obey to is that of reserving upper-case names for MACROs and #defines, every programmer will expect all upper-case names to mean that (like HANDLE, which is a #define).

Practical coding issues
The header-guard on a single line is weird, better make it like this:

#ifndef HANGMAN_H
#define HANGMAN_H

You use some non-standard headers, like <conio.h> and <time.h>, you should avoid conio completely and replace <time.h> with the standard header <ctime>

#include <iostream>
#include <fstream>
#include <windows.h>
#include <string>
#include <ctime>

Also, in your header file, you should avoid including headers that you don't need in order to complete the class declaration. The headers that you only need to implement the class functions should be included in the cpp file. As I see it now, the only headers that you need to complete the class declaration are <string> and <windows.h>. All the other headers, you should include them in the cpp file only, after the inclusion of your header file. This is a practical thing, it helps to avoid dependencies between header files and reduces compilation times, it's a habit worth developing early on.

In your destructor, setting the pointers to NULL after deleting them is not necessary. Furthermore, your constructor doesn't initialize some of the pointers, which might cause a crash if the object is immediately destroyed. This is unacceptable. When you write a class, you must ensure that its state (value of all data members) are valid and consistent with each other, at all times. Additionally, if you have a class that holds dynamically allocated memory, you need to also implement a copy-constructor and a copy-assignment operator which is able to allocate new memory and copy the contents, i.e., a deep-copy. You can read my tutorial about that.

Moreover, your Create.. functions allocate new memory for the buffers but it does not delete the old memory (if any). These functions can either only be used one time or used multiple times in which case your program will leak memory. To save yourself all this trouble with dynamic memory, please just use C++ containers and strings instead of these dynamically allocated arrays. In particular, you could use these data members instead:

std::string _letterBuf;
std::vector<std::string> _wordsBuf;
std::string _hiddenWord;

This will be a lot easier to deal with.

You'll notice in the above that I have written the full qualification for the types, that is, I put the std:: in there. This is because a general rule in real-world programming (beyond your C++ 101 course) is that you should never import a namespace in a header file. This means that a header file should never contain a line like this:

using namespace std;

This is because of name pollution. In general, a header file is supposed to be included by many other source files and "foreign" code. If you import a namespace in your header file, you are also importing that namespace into every other piece of code that includes your header file, and that can be a problem if the third party is not expecting that. Basically, you pollute the global namespace by importing into it all the things in the standard libraries, for example. It's a good habit to develop, learn to live without the using namespace std; statement or any other namespace importing, use full qualifications instead. Also, you should eventually learn to put your own code into a namespace as well.

Other than that, there are a few minor things. The seeding with srand (time(NULL)); is non-standard because the result of the time() function is not guaranteed to be an unsigned int that the srand function expects (it is on some compilers) but you need to cast it, as so: srand((unsigned int)time(NULL)); or srand(static_cast<unsigned int>(time(NULL)));.

Then, as Schol-R-LEA said, you should learn to use the isalpha, tolower and toupper functions, and other standard string manipulation functions. That could greatly simplify your implementations.

Design
I'll come back later with some critiques of the design, because there is a lot to be said there too.

Thanks all, i received a good feedback and im willing to start re-designing this piece of code. There is things that i didnt know and i just read some of them.
Ill start with the design of the constructor and the initialization list, here is one of my questions:
i read its good all variables to be in the initialization list, but, for example, 'lenofFile' i have to use the function 'CountLinesOfFile(...) to initialize 'lenofFile to it. Can i use functions inside it and is it good? (I didnt saw any initialization list do show this, but i read just few tutorials).

I read its bad to use conio.h, and i use getch() from that header. If i have a new project that needs such function, is it good i to write it on my own? What would the mass of programmers do?

I read its bad to use conio.h, and i use getch() from that header. If i have a new project that needs such function, is it good i to write it on my own? What would the mass of programmers do?

getch() is "bad" because it's non-portable. Whether it's actually bad in a practical sense depends on why you use it. For example, one of the classic clueless programmer red flags is using getch() to pause the program:

#include <iostream>
#include <conio.h>

int main()
{
    std::cout << "Hello, world!\n";
    getch();
}

On top of being only conditionally necessary[1], this destroys portability by limiting the code to compilers that support both conio.h and getch(). It's generally seen as stupid by clueful people to decrease portability without good reason. Another fine example of unnecessary loss of portability (and also conveniently another clueless programmer red flag) is the supremely arrogant and idiotic concept of starting a program with clrscr():

#include <iostream>
#include <conio.h>

int main()
{
    clrscr();
    std::cout << "Hello, world!\n";
}

However, if you're using getch() to legitimately accept raw input where the functionality of the program depends on not buffering said input and cannot be suitably replaced with a portable solution, that's fine. One example would be a password input function where you replace each typed character with an asterisk. There's no way to portably create that effect, so when you have no choice but to use a non-portable solution, the portability criticism goes away.

[1] The conditions in which pausing the program at the end would be necessary are when running through an IDE that doesn't keep the owned window open or running the program in such a way that the process owns the window, such as double clicking on the executable's icon in Windows instead of running it through the command prompt.

commented: Excellent explanation... +14

What would the mass programmers do if they need a function like getch()? would they code their own function, what whoud you do guys?

Once again it depends. If I'm only ever planning on using a compiler where getch() is supported, it's silly to write something custom. I'd favor separating it out into a wrapper so that porting would be easier should it be needed though. So first a header with a portable definition:

// raw_input.h
#ifndef RAW_INPUT_H
#define RAW_INPUT_H

int raw_input();

#endif

Then an implementation that uses the non-portable library:

// raw_input.cpp
#include "raw_input.h"
#include <conio.h>

int raw_input()
{
    return getch();
}

This way I could use raw_input() in the main application, and if the time comes where getch() is no longer suitable, I can simply replace the implementation of raw_input() instead of replacing every instance of getch() throughout the entire program. For cases such as this where the wrapper is trivial, there's a good cost to benefit ratio even if the likelihood of porting is fairly low.

The answer would depend on what you needed it for. The first question to really ask is, do you really need it in the first place? As Deceptikon said, most of the time, it is used - or rather abused - for a purpose that could be easily avoided.

In your case, the reason you want to capture a single character entry is to ensure that you get a single letter value, and ignore any other input, correct? Well, unfortunately, there isn't any portable way to do this, as different operating systems handle the console differently. Again, as Deceptikon points out, that is a case where using the <conio.h> library is a reasonable option, especially since the major alternatives - using the Win32 Console API directly, or re-writing the program as a GUI application - would require considerably more effort. You might consider doing either of those later, as you would surely learn quite a bit about Windows programming while doing so and frankly either are preferably to futzing about with something as obsolete as conio, but for the time being what you have isn't too unreasonable.

I would recommend breaking the user interface out from the game proper, however, as it would make it a lot easier to fix problems later, and would give you the flexibility to go from a console program to a GUI program with relative ease. You generally want to separate what is called the view or presentation part of the program from the model and controller parts, to make the program more modular. Even in a small program like this, you would benefit from having a separate internal and external representation of the game.

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.