12
\$\begingroup\$

I wrote a Hangman game to try out some of the new features in C++11. I'm pretty new to C++, and I would like some good advice on how I can improve this code (in terms of conventions, bad/good practices, ...):

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <stdexcept>
#include <random>
#include <algorithm>


const unsigned initial_number_of_lives = 5;

// Returns a random word from the given words file.
std::string pick_word(const std::string& words_filename) {
  std::vector<std::string> words;
  std::ifstream words_file(words_filename.c_str());
  if (!words_file) {
    throw std::runtime_error("Couldn't open file.");
  }
  std::string word;
  do {
    std::getline(words_file, word);
    words.push_back(word);
  } while (!words_file.eof());

  std::uniform_int_distribution<uintmax_t> random_distribution(0, words.size() - 1);
  std::mt19937 random_engine(static_cast<std::mt19937::result_type>(std::time(nullptr)));
  uintmax_t word_index = random_distribution(random_engine);
  return words[word_index];
}

int main(int argc, const char *argv[]) {
  if (argc != 2) {
    std::cerr << "usage: " << argv[0] << " <words-file>\n";
    return 1;
  }

  std::string word;
  try {
    word = pick_word(argv[1]);
  } catch(...) {
    std::cerr << argv[0] << ": couldn't open words file: " << argv[1] << '\n';
    return 1;
  }

#if DEBUG
  std::cout << "(debug) word: " << word << "\n\n";
#endif

  std::cout << "Welcome to hangman!"; // Newline in main loop below.

  unsigned lives = initial_number_of_lives;
  std::vector<char> guessed_letters;
  for (;;) {
    bool won = true;
    for (char &letter : word) {
      if (std::find(guessed_letters.begin(),
                    guessed_letters.end(),
                    letter) == guessed_letters.end()) {
        won = false;
      }
    }
    if (won) {
      std::cout << "You won the game!!1\n";
      return 0;
    }

    std::cout << "\n\nLives left: " << lives;

    std::cout << "\nAlready guessed: ";
    for (char &letter : guessed_letters) {
      std::cout << letter << ' ';
    }

    std::cout << '\n';
    for (char &letter : word) {
      if (std::find(guessed_letters.begin(),
                    guessed_letters.end(),
                    letter) != guessed_letters.end()) {
        std::cout << letter;
      } else {
        std::cout << '_';
      }
    }

    char guess;
    std::cout << "\nEnter a letter: ";
    std::cin >> guess;
    guess = std::tolower(guess);
    // Don't allow player to enter the same letter twice.
    if (std::find(guessed_letters.begin(),
                  guessed_letters.end(),
                  guess) != guessed_letters.end()) {
      std::cout << "You have already guessed that letter!\n";
      continue;
    }

    // Don't allow player to enter anything but letters.
    if (!std::isalpha(guess)) {
      std::cout << "That is not a letter!\n";
      continue;
    }

    guessed_letters.push_back(guess);

    bool word_contains_letter = false;
    for (char &letter : word) {
      if (letter == guess) {
        word_contains_letter = true;
        break;
      }
    }

    if (word_contains_letter) continue;
    --lives;
    if (lives == 0) {
      std::cout << "Game over! The right word was: " << word << '\n';
      break;
    }
  }

  return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ missing include #include <ctime> \$\endgroup\$
    – sehe
    Apr 28, 2012 at 21:35

3 Answers 3

10
\$\begingroup\$

A few thoughts:

Instead of

do {
    std::getline(words_file, word);
    words.push_back(word);
} while (!words_file.eof());

You should do

while(std::getline(words_file, word))
    words.push_back(word);
}

Otherwise, the read which causes the stream to go into an invalid state will still be pushed.

Also, why not include the filename that cannot be opened in the exception text and then output the exception text instead of the hard-coded text you have now. As things are, you may be ignoring other exceptions (although I don't see when one would happen). Perhaps output the exception.what() in addition to what you have now.

I would also advise splitting some of the game logic into functions -- you don't need them in multiple places as things are, but it could be easier to follow. Things like the piece setting word_contains_letter would look better as function calls, in my opinion.

Oh, another thing: you may want to use #ifndef NDEBUG rather than #ifdef DEBUG, as that is the default way to check whether debugging is enabled or not.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Its not probably. He must. Your version works his version is broken and will push the last word into the list twice. \$\endgroup\$ Jan 1, 2012 at 23:57
  • \$\begingroup\$ Removed the probably. :) \$\endgroup\$
    – Komi Golov
    Jan 2, 2012 at 0:02
7
\$\begingroup\$

Here's my inline edits. The most conspicuous result is

  • less manual work
  • 20%-25% shorter (in lines, words, and characters). Less code means less bugs and less maintenance burden (if not obfuscating)

Notes

  1. removed some not very valuable error handling
  2. don't pass filename as std::string for no reason (pass const char*const)
  3. added input checking (just alpha-only words)
  4. removed silly uniform distribution on a one-shot random selection
  5. lowercase picked word
  6. made word const (see below)
  7. replace all range-based fors with the corresponding algorithm (std::copy, std::set::find, std::transform, std::all_of)
  8. use a set for guessed letters
  9. shortened form; I think e.g.

    if (std::string::npos != word.find(guess))
        continue;
    

    is by definition more readable than

    bool word_contains_letter = false;
    for (char &letter : word) {
      if (letter == guess) {
        word_contains_letter = true;
        break;
      }
    }
    
    if (word_contains_letter) continue;
    
  10. there was the matter of style on loops (like the for-loop just above). Consider doing for (const auto&letter : word) or for (char letter : word) so as to make it clear that letter shall not be modified by the loop.

Note I'm not saying my code is (much) better, but it should give you food for thought, which, I think, is the objective of the game.

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <stdexcept>
#include <random>
#include <algorithm>
#include <ctime>
#include <iterator>
#include <set>


const unsigned initial_number_of_lives = 5;

// Returns a random word from the given words file.
std::string pick_word(const char* const words_filename)
{
    std::vector<std::string> words;
    std::ifstream words_file(words_filename);
    std::string word;
    while(std::getline(words_file, word))
        if (std::all_of(word.begin(), word.end(), [](char c) { return std::isalpha(c); }))
            words.push_back(word);

    std::mt19937 random_engine(std::time(nullptr));
    word = words[random_engine() % words.size()];
    std::for_each(word.begin(), word.end(), [](char&c) { c = std::tolower(c); });
    return word;
}

int main(int argc, const char *argv[])
{
    if (argc != 2)
    {
        std::cerr << "usage: " << argv[0] << " <words-file>\n";
        return 1;
    }

    const std::string word = pick_word(argv[1]);
    std::string result(word.size(), '_');

    std::cout << "Welcome to hangman!"; // Newline in main loop below.

    std::set<char> guessed;
    unsigned lives = initial_number_of_lives;

    for (;;)
    {
        std::transform(word.begin(), word.end(), result.begin(), [&] (char c) 
                { return guessed.end()==guessed.find(c)? '_':c; });

        if (result == word)
        {
            std::cout << "\nYou won the game!! \n";
            return 0;
        }

        std::cout << "\n\nLives left: " << lives;

        std::cout << "\nAlready guessed: ";
        std::copy(guessed.begin(), guessed.end(), std::ostream_iterator<char>(std::cout, " "));
        std::cout << '\n' << result << "\nEnter a letter: ";

        char guess;
        std::cin >> guess;
        guess = std::tolower(guess);

        // Don't allow player to enter anything but letters.
        if (!std::isalpha(guess))
        {
            std::cout << "That is not a letter!\n";
            continue;
        }

        // Don't allow player to enter the same letter twice.
        if (guessed.find(guess) != guessed.end())
            continue;

        guessed.insert(guess);

        if (std::string::npos == word.find(guess) // word doesn't contain letter
                && !lives--)                      // no more lives
        {
            std::cout << "Game over! The right word was: " << word << '\n';
            break;
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Your pick_word is reusing a variable (word). I’d flag that in a code review. Also, the implicit int => char coercion in the last conditional is evil and should be replaced by an explicit comparison (lives-- == 0). C++’ habit of treating numbers and logical values identical is annoying. \$\endgroup\$ Apr 28, 2012 at 23:37
  • \$\begingroup\$ @KonradRudolph You meant int => bool? And C++ doesn't treat numbers and logical values as identical. It just does implicit conversions (and stays compatible with C). My stance is, the usual idioms are widely recognized and hence, the 'simpler' code will be easier to grok (and verify). Of course, don't use this in wildly complicated settings involving bit masks etc. Footshooting material there \$\endgroup\$
    – sehe
    Apr 29, 2012 at 0:03
  • \$\begingroup\$ Good point about the 'word' reuse. I wouldn't flag it, but hey, I wouldn't object if anyone else did. \$\endgroup\$
    – sehe
    Apr 29, 2012 at 0:03
  • 1
    \$\begingroup\$ You can usually use std::any_of if you're not actually going to use the iterator returned from find. \$\endgroup\$
    – YoungJohn
    Dec 16, 2013 at 16:42
5
\$\begingroup\$
  1. Consider using conventional ALL_CAPS for constant initial_number_of_lives.

  2. Consider moving the definition of initial_number_of_lives into its minimal scope (inside main, just before using it). The file preamble is really better only for constants that are used in multiple functions.

  3. Consider scalability of pick_word. It doesn't really need to hold every word in the word file in memory at the same time just to fairly pick one. It could use an iterative process with repeated random picks among the words seen so far. Fairness could be maintained by giving the prior pick added weighting equal to the total number of entries it is "replacing" so far.

    This is guaranteed to be fair by the rules of conditional probability. In theory, this process only requires 2 strings in memory at a time, if you don't mind doing n-1 weighted random picks. A reasonable performance compromise, still fair and just slightly more complex, would be to keep small batches of strings in memory (plus the one "weighted" word picked from all of the prior batches) and do fewer random picks, (n-1)/m, where m is the (constant or average) batch size.

  4. Consider unit testability -- it would be easier to unit test the code if pick_word delegated to simple functions like:

    • a separately testable (mockable) random number generator
    • a separately testable (mockable) word picker that operated on an abstract input stream (testable without requiring file access)
  5. Instead of:

    std::vector<char> guessed_letters;
    

    and

    if (std::find(guessed_letters.begin(),
                  guessed_letters.end(),
                  letter) == guessed_letters.end()) {
    

    consider

    std::string guessed_letters;
    

    and

    if (guessed_letters.find(letter) != std::string::npos) {
    

    and even so, consider factoring THAT bit of ugliness into a bool test function. If you took Anton's advice and have a word_contains_letter function, it's already there, ready to be called! And that can be unit tested, too.

    Aside: I try to avoid or at least isolate into a one-liner wrapper function any idiom of the form

    std::METHOD(CONTAINER.begin(), CONTAINER.end(), ARGS) COMPARATOR CONTAINER.end())
    

    Yes, the idioms are so common that experienced STL users can write and read them in their sleep, but still it's ugly! Cxx11 is especially helpful in providing ways to avoid these idioms using higher order programming or to wrap them generically with your own function templates like:

    bool contains(const C& container, const T& element)
    

    I don't know why I can't find sugar like this is in the standard container or algorithm library (have I missed something?).

  6. Consider placing the test for a non-alpha guess BEFORE the check for a repeated guess, just because as it stands it made me think, when I needn't have.

  7. Consider replacing the "forever" loop with the more indicative

    while (lives) {
    

    and extracting the "Game Over" code path from the loop, so

        if (word_contains_letter) continue;
        --lives;
        if (lives == 0) {
            std::cout << "Game over! The right word was: " << word << '\n';
            break;
        }
    }
    

    becomes:

        if (! word_contains_letter) {
            --lives;
        }
    }
    
    std::cout << "Game over! The right word was: " << word << '\n';
    
  8. Consider defending against unwinnable games. If the guesser is restricted to guessing letters, it seems only sporting that the word chosen from the word file be guaranteed to include only guessable letters. Actually, more flexible variant of the game might allow non-letters (spaces, punctuation) in the words (phrases, really) and just reveal them with no effect on the guessing or scoring.

    Any uppercase letters in the words file poses a similar issue, but suggests that you might want to keep a lowercase version of the word for matching guesses but use the original mixed case word for the purposes of showing the guessed letters and revealing the answer at the end. Even if you go with more flexibility in the word list, words that contain an '_' should not be allowed!

  9. Consider keeping yet another version of the word string, a "display" copy, initially filled with ''s. This is more for clarity than performance. It just seems counter-intuitive to be repeating the work of re-filtering the word for display each time through the loop, even when the state of guessed_letters hasn't changed. Each '' in the display copy can be replaced exactly once when that letter is guessed, based on the guessed letter's position(s) in other copy of the word.

    This may lead to the question "Is it OK that word_contain_element calls find, tests the returned position and discards it, and if the test succeeds returns to code that immediately re-executes find with the same arguments (this time at the head of a loop that finds each position of the letter)?" I think the answer is "Yes, because readability is more important than performance, here."

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.