#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;
}
}
-
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\$– 200_successMar 12, 2016 at 19:33
2 Answers
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?
}
-
\$\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\$– KodnotMar 13, 2016 at 11:27
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();