6
\$\begingroup\$

Well this is a basic hangman game in c++ without the classy man getting hanged animation (life is printed instead)

#include <iostream>
#include <cstdlib>
#include <string.h>
#include <time.h>
#include <fstream>

using namespace std;

string previousGuesses;     //string to hold previous guesses

class movie {
    private:
        string name;
        int _size;

    public:
        movie(void);
        inline string getName(void) {
            return this->name;
        }
        inline int getSize(void) {
            return this->_size;
        }
};

movie::movie(void) {
    ifstream fin;
    fin.open("movies.txt", ios::in);
    // int random_integer = rand();
    getline(fin, this->name);       //currently takes the first line as movie name. Randomising logic will be inserted....any hints??
    this->_size = name.size();
    fin.close();
}

int numOfSpaces(string name) {
    register unsigned counter = 0;
    for (register int i = 0; i < name.size(); i++) {
        if(name[i] == ' ') {
            counter++;
        }
    }
    return counter;
}

bool guessedPreviously(char ch) {
    if(previousGuesses.find(ch) != -1) {        //if guess is found, return true
        return true;
    }
    previousGuesses += ch;
    return false;
}

void print(char name[], int size) {
    for (register int i = 0; i < size; i++) {
        cout << name[i];
    }
    cout << endl;
}

int main(int argc, char const* argv[]) {
    /*to get name of the movie*/
    srand(time(NULL)); //seed for rand();
    register int counter = 0;   //used to check if the full guess has been finally made though better logic is welcome
    register int life = 10;     //hold the lives user has
    movie m1;           //object of movie class
    char name[m1.getSize()];    //temp char[] (used to mask/unmask movie name)
    char guess;                 //shall hold user's guess
    string _name = m1.getName();    //a variable to store the movie name (needed as m1.getName()[i] surprisingly doesn't work....why??)
    for (register int i = 0; i < m1.getSize(); i++) {
        /*logic to keep spaces, spaces in the temp char[]*/
        if(_name[i] != ' ')
            name[i] = '*';
        else
            name[i] = ' ';
    }
    while (1) {     //runs the game loop
        /*User's guess*/
        cout << "Enter a guess: ";
        cin >> guess;
        if(guessedPreviously(guess)) {
            cout << "guess already made!!" << endl;
            life--;
            cout << life << endl;
            if (life == 0) {
                cout << "game over!!!" << endl;
                cout << m1.getName() << endl;
                exit(0);
            }
            continue;
        }
        cout << endl;               //I/O formatting ...... because we need it!!
        /*check guess*/
        bool check = false;         //boolean variable to umm.. not let the else if get unnecessarily executed
        for (register int i = 0; i < _name.size(); i++) {
            if(_name[i] == guess) {
                name[i] = guess;    //replaces * in temporary char[] at all places where guess is correct
                check = true;
                /*To check if the full guess has finally been made!!*/
                counter++;
                if(counter == (m1.getSize() - numOfSpaces(m1.getName()))) {     //m1.getSize() also returns spaces, which are not being guessed
                    cout << "congrats!! You've won" << endl;
                    exit(0);
                }
            }
            else if (i == _name.size() - 1 && !check) {     //the 'what to do if guess is wrong??' logic
                cout << "wrong guess!!" << endl;
                life--;     //kill the user once XD
                cout << "You have " << life << " guesses left!!" << endl;
                if (life == 0) {
                    cout << "game over!!!" << endl;
                    cout << m1.getName() << endl;
                    exit(0);
                }
            }
        }
        /*unmask*/
        print(name, m1.getSize());
    }

    return 0;
}

All kinds of improvements are welcome :)

Also how do I make the code case insensitive efficiently??

BTW, the repository is public here

\$\endgroup\$
  • \$\begingroup\$ @dfhwze, Will you please explain your edit? I really didn't find any difference. What was wrong? I am asking this so that I keep these things in mind next time I ask question :). Thanks a lot for editing. Awaiting your reply \$\endgroup\$ – d4rk4ng31 Sep 7 at 7:50
  • 2
    \$\begingroup\$ I added the tag game which is the generic tag accompanying any specific game tag. You don't see this change at the revision (codereview.stackexchange.com/revisions/227619/3) But you do see it at the revision overview (codereview.stackexchange.com/posts/227619/revisions). \$\endgroup\$ – dfhwze Sep 7 at 7:53
  • \$\begingroup\$ Oh!! Thank you. Please don't get offended by the comment I made (though it seems a bit rude 😅) Thanks a lot for pointing out the necessary edits \$\endgroup\$ – d4rk4ng31 Sep 7 at 7:59
  • 1
    \$\begingroup\$ @Mast I am not sure whether this is as designed or not. But I'm not concerned about it either :) If it's a bug, it's hypo minor :p \$\endgroup\$ – dfhwze Sep 7 at 8:06
  • 2
    \$\begingroup\$ Welcome to Code Review! Similar questions are fine on Code Review. \$\endgroup\$ – L. F. Sep 7 at 8:14
4
\$\begingroup\$

Welcome to Code Review! Here's some suggestions.

Overall design

You defined a class movie. Despite its name, all it does is read a string from the configuration file movies.txt. This is unnecessary — just write a function that reads a string.

The global variable previousGuesses contains the previous guesses. This shows that the game is stateful. This is a good use for classes:

class Hangman_game {
public:
    // ...
private:
    std::string word;
    std::string guesses;
};

The numOfSpaces function can be replaced by std::count. The print function is unnecessary. And it is counter-intuitive that guessedPreviously modifies the previousGuesses.

You are using char arrays all over the place. In C++, prefer std::string for actual strings.

Code

#include <iostream>
#include <cstdlib>
#include <string.h>
#include <time.h>
#include <fstream>

Use <cstring> and <ctime>. And sort the headers alphabetically:

#include <cstdlib>
#include <cstring>
#include <ctime>
#include <fstream>
#include <iostream>
using namespace std;

Do not use using namespace std;. It is considered bad practice and will cause a lot of problems. For example, you will have some trouble using common identifiers like size.

class movie {
    private:
        string name;
        int _size;

    public:
        movie(void);
        inline string getName(void) {
            return this->name;
        }
        inline int getSize(void) {
            return this->_size;
        }
};
  1. inline is redundant in a class.

  2. (void) is C-style and is not recommended in C++. Use () instead.

  3. this-> is unnecessary.

  4. std::string maintains the size, so _size should be removed.

public: and private: are usually not indented. And as I said before, this class should not exist at all.

movie::movie(void) {
    ifstream fin;
    fin.open("movies.txt", ios::in);
    // int random_integer = rand();
    getline(fin, this->name);       //currently takes the first line as movie name. Randomising logic will be inserted....any hints??
    this->_size = name.size();
    fin.close();
}

The same thing with compact code:

std::ifstream fin{"movies.txt"}; // the default is "in"
std::getline(fin, name);

Also, "movies.txt" should not be hardcoded.

int numOfSpaces(string name) {
    register unsigned counter = 0;
    for (register int i = 0; i < name.size(); i++) {
        if(name[i] == ' ') {
            counter++;
        }
    }
    return counter;
}

The keyword register is completely ignored by the compiler. Also, use

std::count(name.begin(), name.end(), ' ')

instead.

bool guessedPreviously(char ch) {
    if(previousGuesses.find(ch) != -1) {        //if guess is found, return true
        return true;
    }
    previousGuesses += ch;
    return false;
}

Use std::string::npos instead of -1. The latter is counter-intuitive and may cause signedness warnings.

void print(char name[], int size) {
    for (register int i = 0; i < size; i++) {
        cout << name[i];
    }
    cout << endl;
}

You should never be using a char[], but even if that is the case, use std::cout << std::string_view(name, size) << '\n';. Don't use std::endl.


The main function is too long and looks chaotic. It should be broken into different functions. And you are using a variable length array which is a non-standard extension and should not be used. std::string should be used instead. And there's quite a few problems. rand is infamous for its low quality and should not be used. Too many variables make the logic hard to understand. Most of the loops should be replaced by standard algorithms. return is better than exit(0). Use ++i instead of i++ in a discarded value expression.

Improved version

Here's a very rough version in which I fix the aforementioned problems, not tested extensively:

#include <algorithm>
#include <iomanip>
#include <iostream>
#include <limits>
#include <stdexcept>
#include <string>
#include <string_view>

class Hangman {
public:
    Hangman(std::string_view w, int l)
        :word{w}, current(w.size(), '_'), lives{l}
    {
        if (w.find('_') != w.npos) {
            throw std::runtime_error{"underscore character not supported"};
        }
    }
    void run();

private:
    std::string word;
    std::string current; // the current state (e.g., "_an_man")
    int lives;           // number of lives left

    enum class State {
        none, win, fail
    };

    void display();
    State guess();
};

void Hangman::run()
{
    State state;
    do {
        display();
        state = guess();
    } while (state == State::none);

    display();
    if (state == State::win) {
        std::cout << "Congratulations!  You win.\n";
    } else {
        std::cout << "Game over.\n"
                  << "The word is " << std::quoted(word) << '\n';
    }
}

void Hangman::display()
{
    std::cout << "Word: " << current << "\n"
                 "Lives left: " << lives << "\n\n";
}

auto Hangman::guess() -> State
{
    std::cout << "Guess a letter: ";

    char c;
    std::cin >> c;
    if (!std::cin) {
        throw std::runtime_error{"input failure"};
    }

    if (current.find(c) != current.npos) {
        std::cout << "You have already guessed this letter.\n\n";
        --lives;
    } else if (word.find(c) == word.npos) {
        std::cout << "Bad guess.\n\n";
        --lives;
    } else {
        for (std::size_t i = 0; i < word.size(); ++i) {
            if (word[i] == c)
                current[i] = c;
        }
        if (word == current)
            return State::win;
    }

    if (lives == 0)
        return State::fail;
    else
        return State::none;
}

int main()
{
    Hangman game{"derivative", 6}; // configuration
    game.run();
}

Example session:

Word: __________
Lives left: 6

Guess a letter: a
Word: _____a____
Lives left: 6

Guess a letter: b
Bad guess.

Word: _____a____
Lives left: 5

Guess a letter: c
Bad guess.

Word: _____a____
Lives left: 4

Guess a letter: d
Word: d____a____
Lives left: 4

Guess a letter: e
Word: de___a___e
Lives left: 4

Guess a letter: f
Bad guess.

Word: de___a___e
Lives left: 3

Guess a letter: g
Bad guess.

Word: de___a___e
Lives left: 2

Guess a letter: h
Bad guess.

Word: de___a___e
Lives left: 1

Guess a letter: i
Word: de_i_a_i_e
Lives left: 1

Guess a letter: j
Bad guess.

Word: de_i_a_i_e
Lives left: 0

Game over.
The word is "derivative"
\$\endgroup\$
  • 1
    \$\begingroup\$ @d4rk4ng31 You are welcome. Essentially, the game needs to keep track of some states (the word, the current guessed letters, lives left), so it is advised to use a class to encapsulate them. \$\endgroup\$ – L. F. Sep 7 at 9:09
  • 1
    \$\begingroup\$ @d4rk4ng31 As I said before, rand() is a low-quality RNG and should not be used. C++11 offers the <random> header which contains high-quality RNG. \$\endgroup\$ – L. F. Sep 7 at 9:15
  • 1
    \$\begingroup\$ @d4rk4ng31 register is specified to have no meaning since C++11, and "poisoned" since C++17 (i.e., using it is ill-formed) \$\endgroup\$ – L. F. Sep 7 at 9:27
  • 1
    \$\begingroup\$ @d4rk4ng31 Good optimizing compilers will do a better job of allocating registers than the author (supposedly). Good optimizing compilers may modify the algorithm as well. \$\endgroup\$ – pacmaninbw Sep 9 at 16:02
  • 1
    \$\begingroup\$ @d4rk4ng31I Yeah, it still works in C11 AFAIK, but please note that the notion of "register" doesn't exist in the C or C++ standards, so it exists merely as a recommendation to the implementation. The recommendation isn't always respected, anyway - the implementation is free to put a non-register variable in registers, and free to put a register variable outside registers. \$\endgroup\$ – L. F. Sep 10 at 1:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.