5
\$\begingroup\$

I have the following C++ program:

#include <iostream>
#include <cstdlib>
#include <ctime>
#include <vector>

void printVector (std::vector<int>& vec)
{
    for (int a = 0; a < vec.size(); a++)
        std::cout << (char)vec[a] << ", ";
}

int main ()
{
    const std::string HANGMAN[] = {"\n------+",
                                  "\n      |\n      |\n      |\n      |\n      |\n------+",
                                  "\n   ---+\n      |\n      |\n      |\n      |\n      |\n------+",
                                  "\n   ---+\n   |  |\n      |\n      |\n      |\n      |\n------+",
                                  "\n   ---+\n   |  |\n   O  |\n      |\n      |\n      |\n------+",
                                  "\n   ---+\n   |  |\n   O  |\n   |  |\n      |\n      |\n------+",
                                  "\n   ---+\n   |  |\n   O  |\n  /|  |\n      |\n      |\n------+",
                                  "\n   ----\n   |  |\n   O  |\n  /|\\ |\n      |\n      |\n-------",
                                  "\n   ----\n   |  |\n   O  |\n  /|\\ |\n  /   |\n      |\n-------",
                                  "\n   ----\n   |  |\n   O  |\n  /|\\ |\n  / \\ |\n      |\n-------"};
    const std::string WORDS[] = {"zigzagging", "wigwagging", "grogginess", "beekeeping", "mummifying",
                                 "fluffiness", "fulfilling", "shabinness", "revivified", "kobnobbing",
                                 "beekeepers", "wheeziness", "shagginess", "sleeveless", "parallaxes",
                                 "woolliness", "chumminess", "skyjacking", "grubbiness", "wobbliness",
                                 "feebleness", "jaywalking", "alkalizing", "blabbering", "overjoying"};
    srand(time(0));
    std::string compWord = WORDS[rand() % 25];
    char compHiddenWord[] = "----------";
    int hangmanPos = 0;
    std::vector<int> guessed;
    char userGuess;
    bool letterCorrect;
    while (hangmanPos != 9)
    {
        std::cout << "\nThis is your hangman:\n" << HANGMAN[hangmanPos] <<
                     "\n\nThese are the letters you've already guessed:\n\n";
        printVector(guessed);
        std::cout << "\n\nThis is my word:\n\n" << compHiddenWord <<
                     "\n\nGuess a letter: ";
        std::cin >> userGuess;
        guessed.push_back(userGuess);
        letterCorrect = false;
        for (int a = 0; a < compWord.size(); a++)
            if (compWord[a] == userGuess)
            {
                if (letterCorrect == false)
                {
                    std::cout << userGuess << " is in my word!\n\n\n";
                    letterCorrect = true;
                }
                compHiddenWord[a] = userGuess;
            }
        if (letterCorrect == false)
        {
            std::cout << "Oops! " << userGuess << " is not in my word.";
            hangmanPos++;
        }
        else
            for (int a = 0; a < compWord.size(); a++)
                if (compWord == compHiddenWord)
                {
                    std::cout << "Well done, " << compWord << " was my word!";
                    return 0;
                }
    }
    std::cout << "\nOh Dear! Looks like you've been hanged. My word was actually " << compWord << ".";
}

Which is supposed to replicate the classic game of hangman visually. Is the code fully optimized? Is there an other way I could improve it?

\$\endgroup\$
6
\$\begingroup\$

I'm by far no expert, and I might be wrong on some of my statements, so take them as a basis for discussion. Anyway, I am trying to learn about code quality in C++ myself, so I'm giving it a shot.

  1. printVector should take the argument as const-ref, because the vector is not supposed to be changed in the function call.
  2. Consider using an iterator to loop through the vector in printVector. However, in a small-scale program like this I don't see this as a big issue. In larger programs, it might help you to make a function more general. And thanks to C++11's auto, it will be very readable -- even more so if you use a range-based for loop.
  3. You're using the vector<int> guessed to store the chars from userGuess. Why not a vector<char> to store char? Would also make the type cast to (char) in printVector unnecessary.
  4. Also you're mixing C-arrays and STL's std::vector, C-strings (char *) and std::string. Try to stick to the STL versions throughout. If you are using a C++11 capable compiler, you can even initialize the std::vector the same way as you're doing with the C-array now.
  5. You are using curly braces {} in for loops and if/else statements only when absolutely required. Most style guides encourage using braces for every for/if/else block, or at most allow omitting them for one-line "blocks". The reason is readability (you can always expect to find a } where a longer block ends) and editability (It's easy to forget adding the braces when adding a line to the supposed for/if/else block).
  6. It doesn't look like idiomatic C++ to compare a bool variable with == as in if(letterCorrect == false). You should use something like if (! letterCorrect) instead.
  7. You should add a return 0; statement when the user is hanged. As far as I know it isn't required for the main function, but as you're already returning with return 0; when the user guesses the word, you should also do so at the end of the function.
  8. The condition if (compWord == compHiddenWord) is wrong. You're looping over the array elements, but then you're comparing the pointer to the 0th element of the array to the std::string, instead of comparing the array elements. If you follow my suggestion above and change compHiddenWord to std::string, then you can omit the loop and simply write if(compWord == compHiddenWord) (Edit: As I learned, it also works correctly for comparisons of char* and std::string, so I deleted my example for by-character comparison)
  9. You're using a as a loop index. While this isn't wrong, C++ programmers would typically use i or j for this purpose.
  10. Edit: Your hangman array goes from 0 to 9 -- but in the while loop you never reach the 9 thanks to the hangManPos != 9 condition. Therefore, you will never print the completely hanged hangman.

Let me know what you think!

\$\endgroup\$

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.