HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Hangman game logicistics and cleanliness

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cleanlinesslogicisticshangmangameand

Problem

I recently started working on my code's cleanliness/logistics and would appreciate it if you could give me some tips on how to improve certain things.

Here's a simple console Hangman game I've made:

```
#include
#include
#include
#include
#include
#include
#include

using std::cout; using std::cin;
using std::endl; using std::string;
using std::vector; using std::ifstream;

void Clear_Screen(){
cout Add_Lines(unsigned short int usiLength){
vector vecsLines_Vector;
for(unsigned short int i = 0; i vecsVector, unsigned short int usiLimit){
string sOutput;
for(unsigned short int usiI = 0; usiI > usiMenu_Select;
}while(usiMenu_Select 2);

// Quit Game
if(usiMenu_Select == 2){
return 0;
}
Clear_Screen();

// Grab all the words from the dictionary and store them into a vector
vector vecsWords;
unsigned int uiLength = 0;

ifstream Dictionary;
Dictionary.open("dictionary.txt");
if(Dictionary.is_open()){
while(Dictionary.good()){
string sWord;
getline(Dictionary, sWord);
vecsWords.push_back(sWord);
uiLength++;
}
}
Dictionary.close();

// Pick a random word from the dictionary vector
srand (time(NULL));
const unsigned int kuiRandom_Number = rand() % uiLength;
const string ksWord_Selected = vecsWords[kuiRandom_Number];

// Creates guessing lines
const unsigned short int kusiWord_Length = ksWord_Selected.length();
const unsigned short int kusiTries = 6;
vector vecsGuesses = Add_Lines(kusiWord_Length);
vector vecsWrongs = Add_Lines(kusiTries);

// Guessing game loop
unsigned short int usiIncorrect = 0;
unsigned short int usi

Solution

-
You may group your STL #includes either alphabetically, or by groups (credit to @Loki Astari in a different answer):

// This is Class.cpp
#include "Class.h"
#include "OtherMyClassIdependon1.h"
#include "OtherMyClassIdependon1.h"

// Now C++ header files  // Lots of people order these alphabetically
#include         // Personally I group them
                         // All the containers together.
                         // All the streams together.
                         // etc. Each group alphabetical.

// Now C header files    // Lots of people order these alphabetically
#include 

// Now System header files
#include 


-
Use ` instead of . The latter is a C library and is within namespace std.

-
Those
usings at the top look tacky. I'd just put the std:: where they're needed. This will also aid in separating the STL entities from everything else, for the sake of maintainability and name-clashing avoidance.

-
Functions should start with a lowercase letter if a capital letter is used for types.

Using camelCase:

void oneFunction() {}


Using snake_case:

void another_function() {}


-
Output_Guesses() does not change the container, so pass vecsVector by const&:

std::string Output_Guesses(std::vector const& vecsVector, /* ... */) {}
                                                 // ^^^^^^


-
Move
std::srand() to the top of main() for maintainability and rewrite it like this:

// use nullptr instead if you're using C++11
std::srand(static_cast(std::time(NULL)));


Without this,
std::srand() will return the same random number each time. The casting is also only needed if your compiler gives warnings, so be sure they're enabled.

Also, if you have C++11, replace
NULL with nullptr.

-
When dealing with STL containers (such as
std::vector and std::string), use their iterators instead of indices whenever possible. One occurrence of this is in Add_Lines().

-
For your "winner" and "loser" outputs, you're flushing the buffer at each line, which is what
std::endl is doing. To make a newline without doing this, add \n anywhere within the quotes. You could still put the std::endl at the last line of the program or wherever flushing is needed.

-
while (true) is okay. Another option is for (;;), which also does an infinite loop until stopped (usually with a break).

-
I would consider using a
typedef for your frequently-used types such as const unsigned short int. For instance: if this same type is used for all of your word lengths, then name it something like WordLength. This is capitalized since it's a user-defined type.

Regarding word length, prefer
std::string::size_type. What if you're working with a super freakishly-large string that exceeds the size of an unsigned short int? Having this type will guarantee that you can use such a long word without having to change all of these types.

-
Constants could safely be put in global scope:

const unsigned short int kusiTries = 6


This is okay because it is not a global variable with
const and cannot be modified elsewhere in the program.

Also, refrain from using Hungarian notation in a strongly-typed language such as C++. Just a simple
tries will suffice.

-
With all the "magic numbers" you're sticking in your functions, I'd at least use
EXIT_SUCCESS and EXIT_FAILURE in main() where necessary. As for the "magic numbers" themselves, prefer constants (as mentioned above).

-
For proper inputting with
std::string, prefer std::getline():

getline(std::cin, cGuess);


-
For the input validation, such as for the menu, I'd recommend exception-handling (
try/catch). This would prevent the program from bugging-up from bad input, and would allow recovery.

-
Lastly (and probably most importantly): use more functions. You may still keep the program simple without using classes, and that's alright. However, when you cram everything into
main() (or any one function for that matter), you significantly reduce your program's overall maintainability.

Just keep the "welcome" stuff, menu, and game state (continue playing or not) in
main()`. Everything else should be split into functions based on purpose. It's also not practical to create a function that only displays something as simple as a message, unless it happens to require arguments. Also, if you're using the same block of code repeatedly (such as displaying an array), put that into a separate function.

Code Snippets

// This is Class.cpp
#include "Class.h"
#include "OtherMyClassIdependon1.h"
#include "OtherMyClassIdependon1.h"

// Now C++ header files  // Lots of people order these alphabetically
#include <string>        // Personally I group them
                         // All the containers together.
                         // All the streams together.
                         // etc. Each group alphabetical.

// Now C header files    // Lots of people order these alphabetically
#include <unistd.h>

// Now System header files
#include <ICantThinkOfAnything>
void oneFunction() {}
void another_function() {}
std::string Output_Guesses(std::vector<std::string> const& vecsVector, /* ... */) {}
                                                 // ^^^^^^
// use nullptr instead if you're using C++11
std::srand(static_cast<unsigned int>(std::time(NULL)));

Context

StackExchange Code Review Q#30261, answer score: 14

Revisions (0)

No revisions yet.