8
\$\begingroup\$

I am trying to create a simple Hangman game. The game has an edit mode for first run in which the user has to supply 6 unique words. The game starts by shuffling words, then for every turn, it selects one word that the user has to figure out. The user has only 6 tries to get it right for every word.

How can I improve it?

#include <iostream>
#include <vector>
#include <string>
#include <algorithm>
#include <random>
#include <iterator>
#include <functional>
#include <set>
#include <array>
#include <regex>
#include <limits>

namespace
{
    const std::size_t Max = 6;
    const std::regex Letters("^[A-Za-z]+$");
}

std::mt19937 randomEngine()
{
    std::array<std::mt19937::result_type, std::mt19937::state_size> seed_data;
    thread_local std::random_device source;
    std::generate(seed_data.begin(), seed_data.end(), std::ref(source));
    std::seed_seq seeds(seed_data.begin(), seed_data.end());
    thread_local std::mt19937 seeded_engine(seeds);
    return seeded_engine;
}

template <class InputIt, class Size, class OutputIt, class UnaryPredicate>
void copy_if_n(InputIt first, InputIt last, Size count,
               OutputIt result, UnaryPredicate pred)
{
    if (count <= 0 || first == last) return;

    if (pred(*first)) *result++ = *first;

    for (Size i = 1; i < count && first != last; ++i)
    {
        if (pred(*first)) *result++ = *++first;
    }
}

std::vector<std::string> ask_words()
{
    std::set<std::string> s;
    bool is_good = false;

    while (!is_good)
    {
        std::cout << "Enter 6 unique words: ";
        auto begin = std::istream_iterator<std::string>(std::cin);
        auto end = std::istream_iterator<std::string>();
        copy_if_n(begin, end,
            Max,
            std::inserter(s, s.begin()),
            [](const auto& str)
        {
            return std::regex_match(str, Letters);
        });

        if (std::cin.fail() || s.size() != Max)
        {
            std::cout << "Not valid input.\n";
            if (!s.empty()) s.clear();
            is_good = false;
        }
        else
        {
            is_good = true;
        }

        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }

    return {s.begin(), s.end()};
}

std::string ask_guess(const std::string& str)
{
    std::string result;
    bool is_good = false;

    while (!is_good)
    {
        std::cout << '\n' << str << ": ";
        std::getline(std::cin, result);

        if (std::cin.fail() || !std::regex_match(result, Letters))
        {
            std::cout << "Not valid input.\n";
            if (!result.empty()) result.clear();
            is_good = false;
        }
        else
        {
            is_good = true;
        }

        std::cin.clear();
        std::cin.ignore(std::cin.rdbuf()->in_avail());
    };

    return result;
}

bool askYN(const std::string& str)
{
    char result;
    bool is_good = false;

    while (!is_good)
    {
        std::cout << str << ": ";
        std::cin >> result;
        result = std::toupper(result);

        if (std::cin.fail() || (result != 'Y' && result != 'N'))
        {
            std::cout << "Not valid input.\n";;
            is_good = false;
        }
        else
        {
            is_good = true;
        }

        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    };

    return result == 'Y';
}

int main()
{
    // set up game
    auto words = ask_words();
    thread_local auto engine = randomEngine();
    std::shuffle(words.begin(), words.end(), engine);

    // main loop
    do
    {
        // reset game
        auto word(words.back());
        words.pop_back();
        const auto origin(word);
        std::string score(word.size(), '-');
        std::size_t wrong = 0;
        bool running = true;

        // game loop
        while (running)
        {
            const auto guess(ask_guess("Guess a letter or a word"));

            if (guess.length() > 1) // for long word
            {
                if (guess == origin)
                    std::cout << "\n\nThat's right, you win!\n\n";
                else
                    std::cout << "\n\nWrong word\n\n";

                running = false;
            }
            else // for single letter
            {
                bool found = false;
                const auto ch = guess.back();
                for (std::string::size_type i = 0; i != word.size(); ++i)
                {
                    if (word[i] != ch) continue;
                    score[i] = ch;
                    word[i] = '!';
                    found = true;
                }

                if (!found)
                {
                    std::cout << "\n\nThat's wrong\n\n";
                    wrong++;
                }
            }

            if (wrong == Max)
            {
                running = false;
                std::cout << "\n\nYou Lose! The word was: " << origin << "\n\n";
            }

            std::cout << score << '\n';
            std::cout << "\n\nThere are " << word.length() << " letters left\n";
            std::cout << "You have " << Max - wrong << " more tries left\n\n";

            if (score == origin)
            {
                running = false;
                std::cout << "\n\nYou win!\n\n";
            }
        };
    }
    while (!words.empty() && askYN("Do you want to play again? [y/n]: "));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Do you need to specify std::random_device, std::mt19937 and "engine" as thread_local if you are only using a single thread? \$\endgroup\$ – Kodnot Jun 9 '16 at 8:57
3
\$\begingroup\$

Confusing use of thread_local

thread_local is for variables that can be accessed from multiple threads or have indefinite lifetimes. As you pointed out in your comment, they're implicitly static. But in this code:

std::mt19937 &randomEngine() 
{
    std::array<std::mt19937::result_type, std::mt19937::state_size> seed_data;
    thread_local std::random_device source;
    std::generate(seed_data.begin(), seed_data.end(), std::ref(source));
    std::seed_seq seeds(seed_data.begin(), seed_data.end());
    thread_local std::mt19937 seeded_engine(seeds);
    return seeded_engine;
}

The other three instructions are always executed, even when they are not needed. And the thread_local variables are used like any other variables which makes one forget that they are static.

One way to do things thread local'd (although it's really not needed here, it could all be just static) would be:

std::mt19937 _helper_randomEngine()
{
    std::array<std::mt19937::result_type, std::mt19937::state_size> seed_data;
    std::random_device source;
    std::generate(seed_data.begin(), seed_data.end(), std::ref(source));
    std::seed_seq seeds(seed_data.begin(), seed_data.end());
    std::mt19937 seeded_engine(seeds);
    return seeded_engine;
}

std::mt19937 &randomEngine() {
    /* Could even replace _helper_randomEngine by an anonymous lambda,
       avoiding the external declaration of _helper_randomEngine(); */
    thread_local static engine = _helper_randomEngine();
    return engine;
}

Here the seeding instructions are only ever called once, and the std::random_device is not kept track of as it's unnecessary. The static is extraneous but not harmful, it's up to you whether to keep it or not.

Abuse of std::

This is debatable, but in source files (not headers!) I like doing using namespace std;, especially if it's going to be used a lot like in your source file.

Unreadable bits, Overusage of iterators

This could use some comments:

template <class InputIt, class Size, class OutputIt, class UnaryPredicate>
void copy_if_n(InputIt first, InputIt last, Size count,
               OutputIt result, UnaryPredicate pred)
{
    if (count <= 0 || first == last) return;

    if (pred(*first)) *result++ = *first;

    for (Size i = 1; i < count && first != last; ++i)
    {
        if (pred(*first)) *result++ = *++first;
    }
}

but it's basically a function that reads n inputs matching the predicate, and copies them to result.

But the way it's called:

    auto begin = std::istream_iterator<std::string>(std::cin);
    auto end = std::istream_iterator<std::string>();
    copy_if_n(begin, end,
        Max,
        std::inserter(s, s.begin()),
        [](const auto& str)
    {
        return std::regex_match(str, Letters);
    });

It makes the whole thing seems overly complicated and feel like you forced yourself to use std::copy.

You could have done it much simpler:

do {
   string str;
   cin >> str;

   if (str.length() > 0 && regex_match(str, Letters)) {
       s.insert(str);
   }
} while (s.size() < Max && !cin.fail());

This also has the benefit to wait until 6 different strings are inputted.

Duplication of code

    std::cin.clear();
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

Or

    std::cin.clear();
    std::cin.ignore(std::cin.rdbuf()->in_avail());

Shows several times. Putting it in an explicit function called clear_input() or whatever and calling that function would be better practice.

Confusing auto calls

This is debatable, but things like:

const auto guess(ask_guess("Guess a letter or a word"));

At first read one might think that guess is a function. Why not simply:

const auto guess = ask_guess("Guess a letter or a word");

It's less confusing, and not slower, because of return value optimization. But the const auto is weird here, you might as well do const auto & and the reference will be kept to a temporary that will continue to exist as long as the reference does:

const auto &guess = ask_guess("Guess a letter or a word");

Over complication

I'm going to take the ask_guess function as an example and show the various problems.

std::string ask_guess(const std::string& str)
{
    std::string result;
    bool is_good = false;

    while (!is_good)
    {
        std::cout << '\n' << str << ": ";
        std::getline(std::cin, result);

        if (std::cin.fail() || !std::regex_match(result, Letters))
        {
            std::cout << "Not valid input.\n";
            if (!result.empty()) result.clear();
            is_good = false;
        }
        else
        {
            is_good = true;
        }

        std::cin.clear();
        std::cin.ignore(std::cin.rdbuf()->in_avail());
    };

    return result;
}

First problem:

std::getline(std::cin, result);

Why? Input first accepted strings with >> in ask_words, it's a different behavior here. You're probably doing that so something like "abc def" counts as 1 invalid input and not 2 valid inputs, but if so add a comment explaining why.

Otherwise somebody looking at the code will maybe think you made an error and change the code.

That said, you still do afterwards:

    std::cin.clear();
    std::cin.ignore(std::cin.rdbuf()->in_avail());

So input is cleaned anyway.

Then:

if (!result.empty()) result.clear();

Just do result.clear();. But why do it anyway? It's going to be overwritten in the next reading operation.

Finally, you can remove the boolean flag and have this code:

std::string ask_guess(const std::string& str)
{
    while(1) {
        std::string result;

        std::cout << '\n' << str << ": ";
        std::getline(std::cin, result);

        if (cin.fail() || !std::regex_match(result, Letters)) {
            std::cout << "Not valid input.\n";
            clear_input();
            continue;
        }

        /* Could do RVO if result was declared & returned outside of the loop but move
         * constructor will be called anyway */
        return result;
    };
}

By the way, if something happened to your program that will make cin permanently fail, it'll cause an endless loop. It's better to throw an exception at the first sign of cin failing (especially when reading strings!) and catch it top-level.

I also noticed the lack of std::endl or std::flush, I wonder if there's any reason why.


I didn't analyze the rest of the code, but the summary is that this code feels very academical and not too practical, with some mistakes here and there. At least, it's a lot better than the other extreme (with C-style things everywhere) and you demonstrate knowledge of the different things part of C++.

\$\endgroup\$
  • \$\begingroup\$ thanks for answer, a lot of good points but multi-threads topic is a lot complicated for me, i have posted code in this site earlier but i got different explanations regrading thread_local here link to it: codereview.stackexchange.com/questions/114066/…. according to that answer he mentioned that thread_local is implies static, is that right. \$\endgroup\$ – MORTAL Jun 9 '16 at 18:50
  • \$\begingroup\$ @MORTAL that is right, I will edit my answer accordingly. I still have something to say about that piece of code :p \$\endgroup\$ – coyotte508 Jun 9 '16 at 19:14
  • \$\begingroup\$ @coyotte508 In the rewritten ask_guess is there an issue with not checking cin for eof or bad? Perhaps !cin.good() is more defensive? \$\endgroup\$ – cmh Jun 19 '16 at 2:58
  • \$\begingroup\$ @cmh cin.fail() will detect errors or end of file, so I'm probably not understanding where the problem is. \$\endgroup\$ – coyotte508 Jun 19 '16 at 3:43
  • \$\begingroup\$ @coyotte508 my reading of this doc is that when eofbit is set, but badbit and failbit are not set, then .eof() will return true, but .fail() will return false. Perhaps I'm misreading the docs. \$\endgroup\$ – cmh Jun 19 '16 at 14:47

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.