Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I feel that this implementation can still be simplified. I've tried to keep variable scopes low and have used as much of the standard library as I could.

It currently works with a hard-coded word or phrase, but I eventually plan on having it work with random words or phrases from a file.

My main concern is in regards to my functions. Am I using too few or too many of them? I still feel that main() is doing a little too much.

Note: I have not tagged this as because there's no utilization of it here, but I am open to any recommendations from C++11.

#include <iostream>
#include <string>

bool isCorrectGuess(const char guess, std::string const& word)
{
    return (word.find(guess) != std::string::npos);
}

std::string updateWrongGuesses(const char guess, std::string const& word, std::string wrongGuesses)
{
    return wrongGuesses += guess;
}

std::string updateCorrectGuesses(const char guess, std::string const& word, std::string correctGuesses)
{
    for (std::string::size_type i = 0; i != word.size(); ++i)
    {
        if (word[i] == guess)
        {
            correctGuesses[i] = guess;
        }
    }

    return correctGuesses;
}

void displayStatus(const unsigned int triesLeft, std::string const& guessedWord, std::string const& wrongGuesses)
{
    std::cout << "\n\nRemaining Tries: " << triesLeft;
    std::cout << "\nWrong Guesses  : " << wrongGuesses;
    std::cout << "\n\n" << guessedWord << "\n\n";
}

int main()
{
    const std::string word = "rabbit rabbit";

    std::string guessedWord;

    // set up placeholders for word to be guessed
    for (std::string::size_type i = 0; i != word.size(); ++i)
    {
        guessedWord += (isspace(word[i])) ? ' ' : '*';
    }

    unsigned int triesLeft = 8;
    std::string wrongGuesses;

    displayStatus(triesLeft, guessedWord, wrongGuesses);

    std::string attemptedGuesses;

    while (triesLeft > 0 && guessedWord != word)
    {
        std::cout << "\nGuess a letter: ";
        char guess;
        std::cin >> guess;
        guess = tolower(guess);
        attemptedGuesses += guess;

        if (isCorrectGuess(guess, word))
        {
            guessedWord = updateCorrectGuesses(guess, word, guessedWord);
        }
        else
        {
            wrongGuesses = updateWrongGuesses(guess, word, wrongGuesses);
            triesLeft--;
        }

        displayStatus(triesLeft, guessedWord, wrongGuesses);
    }

    if (guessedWord == word)
    {
        std::cout << "\n\nYou've guessed the word/phrase!\n\n";
    }
    else
    {
        std::cout << "\n\nYou've been hanged...\n\n";
        std::cout << "The word/phrase is: " << word;
    }
}
share|improve this question
    
My personal preference is that main should try as hard as possible to do nothing but utilize other classes or functions. (preprocessor, selection, and loop constructs notwithstanding) –  Casey May 22 at 23:42

2 Answers 2

up vote 5 down vote accepted

The first thing that nearly jumps out at me is this:

std::string updateWrongGuesses(const char guess, std::string const& word, std::string wrongGuesses)
{
    return wrongGuesses += guess;
}

It's not at all clear why you pass word to this at all, since it doesn't use or modify it. Worse, it looks like it's actually more work to call the function than to re-implement what (little) the function does. At least to me:

wrongGuesses += guess;

...seems simpler and more readable than:

 wrongGuesses = updateWrongGuesses(guess, word, wrongGuesses); 

I think I probably would use a class (or possibly two) in writing this code. My first attempt would be something on this general order:

#include <iostream>
#include <string>

class hangman{
    static const int max_wrong = 8;
    std::string word;
    std::string display;
    std::string wrong;
public:
    hangman(std::string const &word) : word(word) {
        for (char ch : word)
            display += isspace(ch) ? ' ' : '*';
    }

    void enter_guess(char ch) {
        bool correct = false;

        for (std::string::size_type i = 0; i != word.size(); ++i)
            if (word[i] == ch) {
                display[i] = ch;
                correct = true;
            }   
        if (!correct)
            wrong += ch;
    }

    bool is_over() {
        return wrong.size() == max_wrong || display == word;
    }

    std::string final() {
        return "The word was: " + word + (display == word ? "\nYou win!" : "\nYou lose");
    }

    std::string status() {
        return "\nwrong guesses: " + wrong + "\nCurrent word: " + display;
    }
};

char get_guess() {
    std::cout << "Please enter your guess: ";
    char ch;
    std::cin >> ch;
    return ch;
}

int main()
{
    hangman game("rabbit rabbit");

    do {
        std::cout << game.status() << "\n";
        game.enter_guess(get_guess());
    } while (!game.is_over());
    std::cout << game.final();
}

As far as how much of your code is in main: I try not to treat main any differently from any other function. IMO, you shouldn't strive for main to be as small, or large, or anything else in particular as possible. Rather, main should be a function just about like any other. It should have accomplish something useful. Like any other function, it should have a clearly defined level of abstraction at which it operates, and (for the most part) it should try to stick to it.

As a natural consequence of being bigger, a bigger program will typically have more layers between main and the lowest levels of manipulation, so in a large program it'll typically have little code beyond creating objects and/or calling functions. I wouldn't consider that a particularly worthy goal in itself though. If it fits well with the size and type of program you're writing great--but there's not necessarily a good reason to force it if it doesn't happen to fit or work out well that way.

share|improve this answer
    
This does look cleaner. I wasn't sure if such a small implementation would've benefited from a Game class, so I just kept it procedural. Or would you say that pretty much any good game implementation should use classes? –  Jamal May 23 at 12:47
    
@Jamal: I'm not sure whether any good game implementation should necessarily use classes. It seemed to work out here, but I haven't really thought it through for other cases. –  Jerry Coffin May 23 at 13:45
    
I suppose I should get familiar with good design practices, then. That's probably one of the things I can really improve on, considering my C++ practices seem decent. –  Jamal May 23 at 13:47

My personal preference is that main should try as hard as possible to do nothing but utilize other classes or functions; preprocessor, selection, and loop constructs notwithstanding.

Because of this policy (it's a little to simple to warrant the use of classes):

  1. The readability is much better and the flow is easier to understand.
  2. It encapsulates changes to the point that the ability to change the hard-coded word to instead use a file to get the word possibly from a list of words and only have to change one function.

#include <iostream>
#include <string>


bool IsCorrectGuess(const char guess, std::string const& word);
void UpdateWrongGuesses(const char guess, std::string const& word, std::string& wrongGuesses);
void UpdateCorrectGuesses(const char guess, std::string const& word, std::string& correctGuesses);
void DisplayStatus(const unsigned int triesLeft, std::string const& guessedWord, std::string const& wrongGuesses);
void ObfuscateWord(const std::string &word, std::string& guessedWord);
void DisplayGameEnd(const std::string& guessedWord, const std::string& word);
char GetGuess();
void AppendAttemptedGuesses(std::string& attemptedGuesses, char guess);
bool IsGameOver(unsigned int triesLeft, const std::string& guessedWord, const std::string& word);
bool IsWordGuessed(const std::string& guessedWord, const std::string& word);
bool HasTriesRemaining(unsigned int triesLeft);
std::string GetWord();
void DecrementGuesses(unsigned int& triesLeft);
bool IsHyphen(int c);
void RunGame(const std::string& word);
void MainLoop(unsigned int guesses_remaining, std::string guessedWord, const std::string& word, std::string& attempted_guesses, std::string& wrongGuesses);

int main() {

    const std::string& word = GetWord();
    RunGame(word);

}

bool IsHyphen(int c) {
    return c == '-';
}

void ObfuscateWord(const std::string &word, std::string& guessedWord) {
    for (std::string::size_type i = 0; i != word.size(); ++i) {
        guessedWord += (isspace(word[i])) ? ' ' : (IsHyphen(word[i]) ? '-' : '*');
    }
}

void DisplayStatus(const unsigned int triesLeft, std::string const& guessedWord, std::string const& wrongGuesses) {
    std::cout << "\n\nRemaining Tries: " << triesLeft;
    std::cout << "\nWrong Guesses  : " << wrongGuesses;
    std::cout << "\n\n" << guessedWord << "\n\n";
}

void UpdateCorrectGuesses(const char guess, std::string const& word, std::string& correctGuesses) {
    for (std::string::size_type i = 0; i < word.size(); ++i) {
        if (word[i] == guess) {
            correctGuesses[i] = guess;
        }
    }
}

void UpdateWrongGuesses(const char guess, std::string const& word, std::string& wrongGuesses) {
    wrongGuesses += guess;
}

bool IsCorrectGuess(const char guess, std::string const& word) {
    return (word.find(guess) != std::string::npos);
}

void DisplayGameEnd(const std::string& guessedWord, const std::string& word) {
    if (IsWordGuessed(guessedWord, word)) {
        std::cout << "\n\nYou've guessed the word/phrase!\n\n";
    } else {
        std::cout << "\n\nYou've been hanged...\n\n";
        std::cout << "The word/phrase is: " << word;
    }
}

char GetGuess() {
    std::cout << "\nGuess a letter: ";
    char guess;
    std::cin >> guess;
    guess = tolower(guess);
    return guess;
}

void AppendAttemptedGuesses(std::string& attemptedGuesses, char guess) {
    attemptedGuesses += guess;
}

bool IsGameOver(unsigned int triesLeft, const std::string& guessedWord, const std::string& word) {
    return (!HasTriesRemaining(triesLeft) || IsWordGuessed(guessedWord, word));
}

bool IsWordGuessed(const std::string& guessedWord, const std::string& word) {
    return guessedWord == word;
}

bool HasTriesRemaining(unsigned int triesLeft) {
    return triesLeft > 0;
}

std::string GetWord() {
    return "rabbit rabbit";
}

void DecrementGuesses(unsigned int& triesLeft) {
    --triesLeft;
}

void RunGame(const std::string& word) {
    std::string guessedWord;
    ObfuscateWord(word, guessedWord);

    unsigned int guesses_remaining = 8;
    std::string wrongGuesses;
    DisplayStatus(guesses_remaining, guessedWord, wrongGuesses);

    std::string attempted_guesses;
    MainLoop(guesses_remaining, guessedWord, word, attempted_guesses, wrongGuesses);

    DisplayGameEnd(guessedWord, word);
}

void MainLoop(unsigned int guesses_remaining, std::string guessedWord, const std::string& word, std::string& attempted_guesses, std::string& wrongGuesses) {
    while (!IsGameOver(guesses_remaining, guessedWord, word)) {
        char current_guess = GetGuess();
        AppendAttemptedGuesses(attempted_guesses, current_guess);
        if (IsCorrectGuess(current_guess, word)) {
            UpdateCorrectGuesses(current_guess, word, guessedWord);
        } else {
            UpdateWrongGuesses(current_guess, word, wrongGuesses);
            DecrementGuesses(guesses_remaining);
        }
        DisplayStatus(guesses_remaining, guessedWord, wrongGuesses);
    }
}
share|improve this answer
    
I could try something like that (after first eliminating all the prototypes). I understand that main() should do little, but I'm not always aware on where to draw the line. –  Jamal May 23 at 1:17
    
@Jamal What's wrong with function declarations? –  Casey May 23 at 1:30
    
Nothing in general, but it could look cluttering if there a lot of functions. They will also have to be maintained along with the function definitions if I change the name and/or arguments. –  Jamal May 23 at 1:33
    
@Jamal That's where code refactoring plug-ins come in handy such as Visual Assist X. Which is exactly what I did to refactor your code, basically highlighted sections and called "Extract Method" :) –  Casey May 23 at 1:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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