Thread: mini game - please feedback on code design

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

.cpp

Code:

#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

Code:

#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

Code:

#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

O_o

You have a required change to your style to make: don't use a leading underscore for variable names; it will eventually bite you on the butt. People who prefer the leading underscore usually move to a trailing underscore, but you are free to do as you like.

Don't use magic numbers; not even for standard characters. It doesn't look nice. It isn't necessary. It is hard to "search and replace".

You are already using the standard C++ library. I can't help but question why you aren't using a `std::vector<???>'.

The input logic is terribly redundant and for that reason prone to errors. The code is essentially `isalpha', but even if you do want to manually code the valid input, the duplication for cases is error prone. You should probably consider using a function that converts the case to one and simple check that one case. Also, consider using a separate function that checks if input is valid beyond the function that records that input.

There is some weirdness in how you've bundled and separated things. The class is partially a driver, terminal wrapper, and game implementation without being all of any of those things. Consider all the benefits of more separation as achieved through interfaces. A separate class for the terminal wrapper, the driver, and the game implementation will give you a lot while costing very little.

Soma

Thanks, 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).

O_o

Your statements and questions make me think you are going to go back towards a flawed monolithic design.

Instead of trying to cram `CountLinesOfFile' and `lenofFile' into this class because "It is convenient to stick everything in one class." consider what you gain by separation of interests.

For example, let's force you to use the C++ standard library to its fullest so that we might use `std::vector<std::string>'. The initialization lists associated with each constructor for your `MyGame' class might call a simple function (a function that is not a part of the class) that returns an instance of `std::vector<std::string>' so that the members of that class can be fully constructed outside of the constructor body.

This doesn't violate any form of "encapsulation" or such other metric. The function that implements the mechanic "Read each line from a text file and store that line in a container." has nothing to do with the logic of "Hangman" and needs no access to private state so has no business being in the class.

Soma

So is good to separate the functions that arent really into hangman game logic and put them in other class? Also if so, how to call such classes, that arent part of the game but 'support' it if i can say it like this.
Hope i understands you

I got some feedback from daniweb also and there saying me to avoid conio header and its functions. Im using getch() in my code.
Is it good to never use conio.h and write his functions on my own? How often you use conio?

I put everything in a single class, just readed a Stroustrop interview and there he said that is better to have not much classes.
I thought if i have a huge project that have many minigames and you are looking for hangman code, to just open its class and find everything regarding to that mini game, inside.

I thought if i have a huge project that have many minigames and you are looking for hangman code, to just open its class and find everything regarding to that mini game, inside.

O_o

Okay. Let's discuss larger code bases in view of a "board game bundle".

You are almost certainly going to need a nice user interface for every game. Are you going to duplicate code (and therefore bugs) for every game? No. You might start out that way, but eventually you'll realize that it was a huge mistake. You will eventually isolate the interface code so that you only need to write and debug it in isolation. You will eventually separate the logic driving the interface (game specific details) into classes that only have a client relationship with the interface code.

*shrug*

Some form of encapsulation is as natural to the human thought process as natural counting; we break large tasks into component tasks so that the details of each component task can be conquered in isolation. That's all we are suggesting here. Yes, you can easily take "Object Oriented Programming" way to far. (A problem I refer to as "Oh. My. God. OOP!".) We aren't suggesting that.

Isolating common utilities so that they might be easily developed, debugged, and used is a solid win. Looking back at my post I suggested moving the "Read each line from a text file and store that line in a container." mechanic into a utility function separate and distinct from the "game" class. I'm not suggesting that such a mechanism needs to be a class; you should just make it a simple function. That utility function can then be used by every "game" in the bundle without you needing to duplicate your code or your effort.

Soma

That might be righ Neo1, i also read that plain old data should be stored in structures. Soma you said that "Read each line from a text file and store that line in a container" isnt a must be in a class, here comes my question. Where to define such lonely functions and structures? Classes have their own .h and .cpp space, if i start to define struct and functions in the main.cpp doesnt it in a large project gonna be a problem?

Yes im working on design and again i wanna make the best habits as possible, no matter much work will cost

Originally Posted by CoffeCat

if i start to define struct and functions in the main.cpp doesnt it in a large project gonna be a problem?

Function declarations can also be in the header.
Put the definition in the .cpp file.
Also, wrap them up in a namespace, so you have a guaranteed freedom from name collisions.

For many small classes see anti-pattern: Functional Decomposition

GRAH!

You have no idea how much it ........es me off that such nonsense became known as "Functional Decomposition".

How does that even happen?

"Every function gets put into a private class by itself."

How is that "Functional Decomposition"?

When did people start redefining centuries old jargon describing an approach to processing large problem spaces into manageable chunks as something describing the dumbest "Oh. My. God. OOPS." (*) nonsense?

Soma

(*) Note: The "Object Oriented Programming" umbrella includes a lot of great tools and techniques. The "Oh. My. God. OOPS." mindset is mindlessly applying those techniques because it is the fad.