Hcfyk

After finishing my C++ course, I decided to make a hangman game. (without the drawing part) I was wondering how the code could be improved.

Header File:

#ifndef GAME_H

#define GAME_H

#include <string>

#include <fstream>

#include <vector>

#include <Windows.h>

class Game

{

public:

    Game();

    int onOffSwitch(std::string);

    void readRandomLine();

    void display();

    void revealLetter(char);

    void revealRandomly();

    void createUnreveal();

    void determineAttempts();

    void countLines();

    void iStreamClear();

    void decrementAttempts();

    void newGame();

    void endGame();

    bool isRevealed(char);

private:

    char z;

    unsigned int attempts = 0;

    unsigned int totalLines = 0;

    std::ifstream inputStream;

    std::string theWord;

    std::string unrevealed;

    bool gameOver = false;

    bool guessedRight;

    HANDLE colorControl;

    std::vector<char> revealed;

};

#endif // !FILE_OPERATIONS_H

Implementation File:

#include "Game.h"

#include <iostream>

#include <fstream>

#include <string>

#include <ctime>

#include <iomanip>

#include <Windows.h>

#include <vector>

#include <algorithm>



// Constructor opens file and invokes newGame function

Game::Game()

{

    onOffSwitch("open");

    newGame();

}



// Function to open file along a fail-check

int Game::onOffSwitch(std::string command)

{

    if (command == "open")

    {

        inputStream.open("words.txt");

        if (inputStream.fail())

        {

            std::cerr << "Error Opening File" << std::endl;

            return -1;

        }

    }

    else if (command == "close")

    {

        inputStream.close();

    }

}



// Function to count number of lines in the file (for purposes of random number generation)

void Game::countLines()

{

    std::string tempS;

    if (inputStream.is_open())

    {

        while (!inputStream.eof())

        {

            getline(inputStream, tempS);

            totalLines = totalLines + 1;

        }

    }

    else

    {

        onOffSwitch("open");

        countLines();

    }

}



// Function that reads a random line(word) from the text file

void Game::readRandomLine()

{

    srand(time(NULL)); // resets

    if (inputStream.is_open())

    {

        int random = 0;

        countLines();

        random = (rand() % totalLines) + 1; // random line to read

        iStreamClear(); // clear EndOfFile flag on ifstream

        int currentLine = 1;

        // While loop to keep reading until we get to the right line

        while (currentLine <= random)

        {

            getline(inputStream, theWord);

            currentLine++;

        }

        determineAttempts();

    }

    else

    {

        onOffSwitch("open");

        readRandomLine();

    }

}



// Function to display the current state of the unrevealed word

void Game::display()

{

    if (gameOver == false)

    {

        for (int t = 0; t < unrevealed.length(); t++)

        {

            std::cout << std::setw(2) << unrevealed[t];

        }

        std::cout << std::endl;

    }

}



// Function that determines number of attempts the player has depending on word length

void Game::determineAttempts()

{

    if (theWord.length() == 4)

    {

        attempts = 2;

    }

    else if (theWord.length() >= 5 && theWord.length() <= 7)

    {

        attempts = 3;

    }

    else if (theWord.length() > 7)

    {

        attempts = 4;

    }

    std::cout << "You have " << attempts << " attempts!" << std::endl;

}



// Function to remove EndOfFile flag and start back in the beginning of the file

void Game::iStreamClear()

{

    inputStream.clear();

    inputStream.seekg(0, inputStream.beg);

}



// Creates an unrevealed version of the random word we read. (with underscores)

void Game::createUnreveal()

{

    unrevealed = theWord;

    for (int r = 0; r < theWord.length(); r++)

    {

        unrevealed[r] = '_';

    }

}



// Reveals a letter randomly

void Game::revealRandomly()

{

    srand(time(NULL));

    int ran = rand() % unrevealed.length();

    revealLetter(theWord[ran]);

}



// Checks and reveals a specific letter

void Game::revealLetter(char l)

{

    guessedRight = false;

    for (int e = 0; e < unrevealed.length(); e++)

    {

        if (theWord[e] == toupper(l) || theWord[e] == tolower(l))

            // The condition includes both upper and lower so that it works with both lowercase and uppercase entries by the player

        {

                guessedRight = true;

                if (e == 0)

                    // If it's the first letter, it should be uppercase

                {

                    revealed.push_back(l); // Puts the letter into a vector for checking if the letter was already revealed

                    unrevealed[e] = toupper(l);

                }

                else

                {

                    revealed.push_back(l); // Same as above

                    unrevealed[e] = tolower(l);

                }

        }

    }

}



// Function to lower attempts if the right conditions are met.

void Game::decrementAttempts()

{

    // Sets console color

    colorControl = GetStdHandle(STD_OUTPUT_HANDLE);

    if (unrevealed == theWord && attempts != 0)

        // If the unrevealed letter is the same as the secret word and player still has attempts, they win

    {

        SetConsoleTextAttribute(colorControl, 13);

        std::cout << theWord << std::endl;

        std::cout << "Congrats! You won!" << std::endl;

        gameOver = true;

        endGame();

    }

    else if (attempts >= 1 && guessedRight != true)

    // If attempts are >= to 1 and they didn't guess right, they lose 1 attempt

    {

        attempts -= 1;

        // If attempts become 0 after the change, then the game is over and endGame function gets called to see if they want to play again

        if (attempts == 0)

        {

            SetConsoleTextAttribute(colorControl, 10);

            std::cout << "No attempts left! Game over!" << std::endl;

            SetConsoleTextAttribute(colorControl, 9);

            std::cout << "The word was " << theWord << "." << std::endl;

            gameOver = true;

            endGame();

        }

        else

        {

            std::cout << "You have " << attempts << " attempts left!" << std::endl;

        }

    }

}



// Function that prompts the player to play again or end the game

void Game::endGame()

{

    char ans;

    revealed.clear(); // clearing the vector so we don't have leftover characters from previous games

    std::cout << std::endl;

    colorControl = GetStdHandle(STD_OUTPUT_HANDLE);

    SetConsoleTextAttribute(colorControl, 4);

    std::cout << "Want to play again? (y/n)";

    std::cin >> ans;

    SetConsoleTextAttribute(colorControl, 15);

    if (ans == 'y' || ans == 'Y')

    {

        std::cout << std::endl;

        newGame();

    }

    else if (ans == 'n' || ans == 'N')

    {

        gameOver = true;

        SetConsoleTextAttribute(colorControl, 6);

        std::cout << "Thank you for playing!" << std::endl;

    }

}





void Game::newGame()

{

    gameOver = false;

    // Clears both words

    theWord.clear();

    unrevealed.clear();



    // Calls all the necessary functions for the game to work

    readRandomLine();

    createUnreveal();

    revealRandomly();

    display();

    // While loop that asks the player for a letter as long as game is not over (either by winning or losing, so no more attempts left)

    while (attempts > 0 && gameOver != true)

    {

        std::cout << "Enter a letter: ";

        std::cin >> z;

        if (isRevealed(z) == true)

        // If the letter is already revealed

        {

            std::cout << "Letter is already revealed!" << std::endl;

            display();

        }

        else

        {

            revealLetter(z);

            decrementAttempts();

            display();

        }   

    }

}



// Checks through the vector to see if the particular letter is already revealed

bool Game::isRevealed(char s)

{

    if (std::count(revealed.begin(), revealed.end(), tolower(s)) == true)

    {

        return true;

    }

    else if (std::count(revealed.begin(), revealed.end(), toupper(s)) == true)

    {

        return true;

    }

    else

        return false;

}

main.cpp :

#include <iostream>

#include "Game.h"

int main()

{

    Game pass;

    pass.onOffSwitch("close");

    return 0;

}

I will proceed from a high level to a low level perspective.

Starting with main.cpp:

Game pass;

pass.onOffSwitch("close");

The meaning of these two lines are not clear without reading the contents of the class Game. That means that not only do I need other pieces of code, but different files, to understand these two lines.

Having looked into the Game class, it becomes apparent that the game, when it is created, starts itself in the constructor. This is not what you would expect from the above code lines.

It would be easier to understand if the constructor were only used to construct the instance, without executing any logic. Viewing the main function as the "user" of the instance, what I usually try to do is formulate what that piece of code is supposed to achieve in words inside of my head, and then translate it to code.

"I want to create an instance of the Hangman game and then start it.", translated to C++:

Game hangman;

hangman.start("words.txt");

Let's look at the constructor:

// Constructor opens file and invokes newGame function

Game::Game()

{

    onOffSwitch("open");

    newGame();

}

The comment tells us that onOffSwitch opens a file, but the code does not indicate that. Opening the file is all it does (apart from some error handling), so lets suppose we rename it to readWordsFromFile. (We will look at the onOffSwitch method in a moment.)

The next thing the comment tells us is that it invokes the method, but that is something the code itself tells you already. It is usually better to only comment why the code does something (if it helps understanding it), but not what it does.

An example of how I would rewrite that piece of code (at this level, not considering further improvements we will look at) is this:

 Game::Game()

 {

     readWordsFromFile();

     newGame();

 }

With the renamed method name, the comment becomes obsolete and can be removed, so the maintainer has less to read for the same understanding.

In the previous section, we renamed the method onOffSwitch. Let's have a look at why that name is not a good fit.

// Function to open file along a fail-check

int Game::onOffSwitch(std::string command)

{

    if (command == "open")

    {

        inputStream.open("words.txt");

        if (inputStream.fail())

        {

            std::cerr << "Error Opening File" << std::endl;

            return -1;

        }

    }

    else if (command == "close")

    {

        inputStream.close();

    }

}

Again the comment above the method is a good indication: Whenever you want to add a comment explaining what a method does, that explanation should probably be its name. What this method does is one of two things, depending on a parameter: Either it opens a file, or it closes it.

The parameter is of type string. That means that if you mistype the parameter, the compiler will not warn you, the method will not tell you that something is wrong, instead nothing will happen (at that point). The bugs will probably occur much later in the program, and will be difficult to track. You could use a boolean or enum instead, which would prevent the typo problem. Even better would be to replace it with 2 methods which do just one thing each: openFile and closeFile, and without parameters to select what they should do.

Most of your methods do not return anything and do not take any parameters. Instead they change the state of the object, which is essentially global state since all of the code resides inside the class and thus has access to it. This can quickly become very complex and bugs will easily be introduced, but hard to find.

Instead of all these methods that access the file stream to open or close it, to count its lines or to select and read a random line, a better approach could be the following:

1. Open the file stream, read all lines into an std::vector<string>, close the file stream.


2. Use the vectors size() method to ask it for the number of lines, and use it for calculating the random index.


3. Using the random index, read directly from the vector of strings, rather than reading through the whole file again.

This way the code becomes more readable, more performant and less prone to bugs. For example you will only have to read the file once, and you will not have to care about when the file stream is opened and closed. You open it once, read it, and close it. (This assumes that the file is not millions of words long, which could become a memory issue, but maybe a few thousand words).

Let's look at the implementation of some of the other methods.

void Game::determineAttempts()

{

    if (theWord.length() == 4)

    {

        attempts = 2;

    }

    else if (theWord.length() >= 5 && theWord.length() <= 7)

    {

        attempts = 3;

    }

    else if (theWord.length() > 7)

    {

        attempts = 4;

    }

    std::cout << "You have " << attempts << " attempts!" << std::endl;

}

Here the implementation does not really fit the method name, while the method name itself is a pretty good choice in my opinion. Instead of calculating the number of attempts, setting some state in the outside worlds (basically global state), and printing to the console, it is better to break things down more into small sub-problems. The sub-problem that this method should solve is this: "Take the word, determine how many attempts the player has, and give me the result."

int Game::determineAttempts(std::string word)

{

    int numberOfAttempts = 0;



    if (word.length() == 4)

    {

        numberOfAttempts = 2;

    }

    else if (word.length() >= 5 && word.length() <= 7)

    {

        numberOfAttempts = 3;

    }

    else if (word.length() > 7)

    {

        numberOfAttempts = 4;

    }



    return numberOfAttempts;

}

This version does not write to the console and it does not change any state. Instead, the caller of the method can decide whether to print something to the console and what to do with the number of attempts the player should have.

Note also that not all possible word lengths are checked. What happens when a word has only three letters (or less)?

All of Game's methods are public, but the purpose of public is to expose them to callers outside of the class itself. onOffSwitch is the only method that is called from outside (namely the main function), so all other methods should be private.

Single Responsibility Principle

The class Game does too much directly, this could be an aggregation of classes instead. One of the points of the Single Responsibility Principle is that a class or function has limited tasks to perform so that it is easier to write, read and debug. The file input should have it's own class that game might evoke. The single responsibility is one of the 5 key principles of SOLID programming. SOLID programming is a good method of Object Oriented programming.

Error Checking

The function onOffSwitch(std::string command) returns an integer value that is always ignored. There is no error checking on the value to see if the file words.txt was opened successfully. If the file doesn't exist the program goes into an infinite loop (Bug number 1).

Don't Ignore Warning Messages

I built and ran this program in Visual Studio 2015. The following warning messages were issued during the build:

1>------ Build started: Project: HangMan1, Configuration: Debug Win32 ------

1>  Game.cpp

1>d:codereviewhangman1hangman1game.cpp(58): warning C4244: 'argument': conversion from 'time_t' to 'unsigned int', possible loss of data

1>d:codereviewhangman1hangman1game.cpp(86): warning C4018: '<': signed/unsigned mismatch

1>d:codereviewhangman1hangman1game.cpp(123): warning C4018: '<': signed/unsigned mismatch

1>d:codereviewhangman1hangman1game.cpp(132): warning C4244: 'argument': conversion from 'time_t' to 'unsigned int', possible loss of data

1>d:codereviewhangman1hangman1game.cpp(141): warning C4018: '<': signed/unsigned mismatch

1>d:codereviewhangman1hangman1game.cpp(257): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation

1>d:codereviewhangman1hangman1game.cpp(261): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation

1>d:codereviewhangman1hangman1game.cpp(34): warning C4715: 'Game::onOffSwitch': not all control paths return a value

========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========

The warning messages should be treated as error messages in this case because it shows possible bugs in the code. The last warning message in particular should be treated as an error message, all paths through the code should always return a value.

Bug Number 2

After words.txt was added the program ran and it picked one of the lines as it was supposed to. When the first letter of the line was added (not the first guess or the first correct guess) the letter was converted to a capital which made the answer when it was entered wrong. It might be better to convert all user input to lower case upon entry.

Portability

The code is not portable because it includes windows.h. It also uses windows only features such as STD_OUTPUT_HANDLE. It might be better to ifdef this code so it can be moved to other platforms.

In your isRevealed method, you're using the count algorithm, which believes that you are interested in knowing the exact number of letters, whereas you just want to know if it is present at least once. More, you are making two complete passesover the letters where a partial pass stopping at the first occurrence would suffice. You could rewrite your function body as:

const auto lowerCaseS = tolower(s);

return std::any(revealed.begin(), revealed.end(), [&](char c) { return tolower(c) == lowerCaseS; });

In your createUnreveal function, you are actually trying to create a string composed of _ alone, that has the same length as theWord. Your function could be simplified as:

unrevealed = std::string{theWord.length(), '_'};