2
\$\begingroup\$
#include <iostream>
#include <string>
#include <algorithm>
#include <time.h>
int faults = 0;
int hangman(); 
int i = hangman();
int main(){


srand(time(NULL));
while (i == 1){
    std::string words[7]{"Alpha", "Cornwall", "Crepuscular", "Blind", "Steroid", "Plunder", "Talisman"};
    std::string word = words[rand() % 7];
    int n;

    n = rand() % word.length();
    char underscore = '_';
    char checkAnswer = word.at(n);
    word.at(n) = underscore;

    std::cout << word;
    char answer;
    std::cin >> answer;


    if (answer == checkAnswer){
        std::cout << "Correct!";
    }
    else{
        faults++;
        hangman();
    }
}






}
      int hangman(){
       if (faults == 0){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|" << std::endl;
        std::cout << "|" << std::endl;
    std::cout << "|" << std::endl;
    return 1;
}
else if (faults == 1){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|" << std::endl;
    std::cout<<"|" << std::endl;
    return 1;
}
else if (faults == 2){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |"<<std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|" << std::endl;
        ;
    return 1;
}

else if (faults == 3){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                     /" << std::endl;
        ;
    return 1;
}
else if (faults == 4){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      X" << std::endl;
    return 1;
}
else if (faults == 5){
    std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        std::cout << "                       -" << std::endl;
    return 1;
}
else if (faults == 6){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      X" << std::endl;
    std::cout << "                      - -" << std::endl;
    return 1;
}
else if (faults == 7){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      O" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|                      X" << std::endl;
    std::cout << "                      - -" << std::endl;
    std::cout << "Dead.Dead.The hangman is dead!You lose!" << std::endl;
    return 0;
}
}
\$\endgroup\$
1
  • 5
    \$\begingroup\$ There are many ways to write Hangman. Could you tell us what makes yours different, such as any specifications or design goals you might have? \$\endgroup\$ Mar 12, 2016 at 19:33

2 Answers 2

6
\$\begingroup\$

First of all, that's not how hangman is usually implimented, but I won't question it, I'll only review your code.

1. Make sure your code actually works

You currently have an endless while loop. Your hangman() function does not do what it's supposed to, it does not update the value of i. Remove the 'i' variable entirely and change the main loop to "while(hangman()) { ... }". You should also remove the hangman() call at the end of your while loop (in the 'else' block), because it's not needed after this adjustment.

(note: that's the least you should do. I'd suggest avoiding global variables entirely (you'll still have a global variable 'faults'), but changing that would require redesigning the whole code. You should at least change 'faults' to 'g_faults' to indicate that it's a global variable)

2. Adjust the output of your program

This is sort of a minor nitpick, but your output looks ugly. A space before user input and a newline char after 'correct!' should fix it.

3. Properly indent your code

Your code as it currently is is hard to read and understand, because it does not follow the common identation rules. I'll post your code with proper indentation later.

4. Don't use the NULL macro

Especialy in this situation, there is no need for it (correct me if I'm wrong). Simply write 'time(0)'.

5. Name your variables properly

The name of your variable should make clear what the varable does. Thus, your 'word' string should be 'current_word' or at least 'cWord', the 'checkAnswer' char should be 'currentLetter' or 'missingLetter' ect.

Sorta fixed code

#include <iostream>
#include <string>
#include <algorithm>
#include <time.h>

int faults = 0;
int hangman();

// No need for 'i' variable

int main(){
    // No need for NULL macro
    srand(time(0));

    // If you declare words[] inside the while loop, it gets created and destroyed every time the loop
    // executes. There is no need for that, thus you should declare the array outside of the loop
    std::string words[7]{"Alpha", "Cornwall", "Crepuscular", "Blind", "Steroid", "Plunder", "Talisman"};

    while (hangman()){
        std::string word = words[rand() % 7];

        // No need to separately declare and initialize the 'n' variable
        int n = rand() % word.length();
        char checkAnswer = word.at(n);

        // No need for an underscore variable, because you only use it once in your code.
        // One could argue that you should still keep the variable in case you wanted to change the
        // symbol, but your variable name indicates that it equals to '_', so you wouldn't use it
        // for that.(What I meant is if you wanted to change "A_pha" to, let's say, "A*pha")
        word.at(n) = '_';

        std::cout << word << ' ';
        char answer;
        std::cin >> answer;

        if (answer == checkAnswer){
            std::cout << "Correct!\n";
        }
        else{
            // Prefer ++a to a++. It doesn't matter in this case, but it's a good practice
            ++faults;
        }
    }
}

int hangman(){
    // Switch statement may have been more fitting here, but I'm too lazy to rewrite
    // your code, so do it yourself if you want to :P
    if (faults == 0){
    std::cout << "|----------------------" << std::endl;
    std::cout << "|                      |" << std::endl;
    std::cout << "|" << std::endl;
    std::cout << "|" << std::endl;
    std::cout << "|" << std::endl;
    return 1;
    }
    else if (faults == 1){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|" << std::endl;
        std::cout<<"|" << std::endl;
        return 1;
    }
    else if (faults == 2){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |"<<std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|" << std::endl;
            ;
        return 1;
    }

    else if (faults == 3){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                     /" << std::endl;
            ;
        return 1;
    }
    else if (faults == 4){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        return 1;
    }
    else if (faults == 5){
        std::cout << "|----------------------" << std::endl;
            std::cout << "|                      |" << std::endl;
            std::cout << "|                      O" << std::endl;
            std::cout << "|                      |" << std::endl;
            std::cout << "|                      X" << std::endl;
            std::cout << "                       -" << std::endl;
        return 1;
    }
    else if (faults == 6){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        std::cout << "                      - -" << std::endl;
        return 1;
    }
    else if (faults == 7){
        std::cout << "|----------------------" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      O" << std::endl;
        std::cout << "|                      |" << std::endl;
        std::cout << "|                      X" << std::endl;
        std::cout << "                      - -" << std::endl;
        std::cout << "Dead.Dead.The hangman is dead!You lose!" << std::endl;
        return 0;
    }
    // Again, just a design nitpick, but your images of hangman with 6 and 7 faults are identical.
    // Shouldn't the hangman be dead after 6 faults then?
}
\$\endgroup\$
1
  • \$\begingroup\$ I might have been mistaken about the 'time(0)' thingy. The NULL macro should still be avoided tho, so if time() takes a null pointer as an argument, you should use nullptr instead of NULL (That is, if you have c++11 support). More about it: [stackoverflow.com/questions/1282295/what-exactly-is-nullptr] \$\endgroup\$
    – Kodnot
    Mar 13, 2016 at 11:27
2
\$\begingroup\$

You've already gotten what I think is some good feedback, so I'll try not to repeat it. I do see a few more things I think are open to improvement though.

Avoid global variables

First, I'd rather pass faults as a parameter to the hangman function rather than using a global variable:

int hangman(int faults) { 
    // ...

Don't Repeat Yourself

Second, I'm not excited about how much repetition there is in the code for the hangman function. I'd rather see the data consolidated into one place, and the logic reflect that of the game itself more closely. Something on this order seems more reasonable to me:

void hangman(int faults) {
    static char const *body[] = {
        "|                     O\n",
        "|                     |\n",
        "|                     X\n",
        "|                    - -\n",
        "Dead.Dead.The hangman is dead!You lose!\n"
    };

    std::cout << "|----------------------\n|                     |\n";

    if (faults > 5)
        faults = 5;

    int i;
    for (i = 0; i < faults; i++)
        std::cout << body[i];
    for (; i < 5; i++)
        std::cout << "|\n";
}

Don't use rand

For some time now (since C++11) we've had better random number generation facilities than srand/rand available. Rather than argue about NULL vs. 0, use them and don't call time at all. Unfortunately, using these as they're provided by the library is somewhat verbose and painful, so I encapsulate one reasonable way to use them in a tiny class:

class generator {
    std::mt19937 rng;
    std::uniform_int_distribution<int> uni;
public:
    generator(int low, int high) : rng(std::random_device()()), uni(low, high) { }
    int operator()() { return uni(rng); }
};

With this, you'd define something like this:

generator gen(0, word.length()-1);

...and generate a number like this:

n = gen();
\$\endgroup\$
0

Your Answer

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

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