C++ Hangman Game, class organization

Question

I have recently created a Hangman Game in C++ and I would like your feedback on any improvements that can be made to this Hangman game I have written especialy in class organization. Here is the code : https://github.com/ydepledt/Hangman. (for now code function only for french words because I haven't yet implemented words from english)

File.hpp

#ifndef FILE_HPP
#define FILE_HPP
#include <iostream>
#include <string>
#include "Settings.hpp"
#include "utils.hpp"
class File {
 public:
 File() = delete;
 File(LANGUAGE language, std::string const & filePath, int nbOfLetters = 5);
 File(Settings settings);
 int nbOfWordsInFile ();
 std::string generateNameFile ();
 std::string generateLanguageFile();
 std::string generateFilePath ();
 std::string getWordFromFile(int position);
 std::string getRandomWord();
 private:
 LANGUAGE m_language;
 int m_nbOfLetters;
 std::string m_filePath;
};
#endif

Game.hpp

#ifndef GAME_HPP
#define GAME_HPP
#include <iostream>
#include <vector>
#include <algorithm>
#include "Word.hpp"
#include "Settings.hpp"
#include "File.hpp"
#include "utils.hpp"
class Game {
 public:
 Game() = delete;
 Game(DIFFICULTY const difficulty, LANGUAGE const language, int const numberOfLetters);
 void initialisation();
 void step();
 bool finished();
 static void askUserSettings(DIFFICULTY & difficulty, LANGUAGE & language, int & numberOfLetters);
 static bool askPlayAgain();
 private:
 int m_count;
 Settings m_settings;
 File m_file;
 Word m_word;
 int m_limit;
};
#endif

Settings.hpp

#ifndef SETTINGS_HPP
#define SETTINGS_HPP
#include <iostream>
#include "utils.hpp"
class Settings {
 public:
 Settings() = delete;
 Settings(DIFFICULTY difficulty, LANGUAGE language = FRENCH, int numberOfLetters = 5);
 DIFFICULTY getDifficulty() {return m_difficulty;};
 LANGUAGE getLanguage() {return m_language;};
 int getNbOfLetters() {return m_numberOfLetters;};
 private:
 DIFFICULTY m_difficulty;
 LANGUAGE m_language;
 int m_numberOfLetters;
};
#endif

utils.hpp

#ifndef UTILS_HPP
#define UTILS_HPP
#include <iostream>
#include <limits>
#include "algorithm"
enum LANGUAGE {FRENCH, ENGLISH};
enum DIFFICULTY {EASY, MEDIUM, HARD, EXTREME};
void stringToLower (std::string & s);
int checkStringBool (std::string s);
int randomNumber (int sup, int inf = 0);
// void createAllFilesTXT (LANGUAGE language, std::string const & bigFilePath);
int transformDifficultyInLimit(DIFFICULTY difficulty);
std::istream& operator>>(std::istream& flow, DIFFICULTY & difficulty);
std::istream& operator>>(std::istream& flow, LANGUAGE & language);
template <typename T, typename Predicate, typename Predicate2>
void secureInput(T & var, Predicate predicate, Predicate2 predicate2)
{
 while (!(std::cin >> var) || !predicate(var) || !predicate2(var))
 {
 if (std::cin.eof())
 {
 throw std::runtime_error("Flow stopped !");
 }
 else if (std::cin.fail())
 {
 std::cout << "Incorrect input" << std::endl;
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 }
 else
 {
 std::cout << "Predicates non respected" << std::endl;
 }
 }
}
template <typename T, typename Predicate>
void secureInput(T & var, Predicate predicate)
{
 while (!(std::cin >> var) || !predicate(var))
 {
 if (std::cin.eof())
 {
 throw std::runtime_error("Flow stopped !");
 }
 else if (std::cin.fail())
 {
 std::cout << "Incorrect input" << std::endl;
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 }
 else
 {
 std::cout << "Predicates non respected" << std::endl;
 }
 }
}
template <typename T>
void secureInput(T & var)
{
 while (!(std::cin >> var))
 {
 if (std::cin.eof())
 {
 throw std::runtime_error("Flow stopped !");
 }
 else if (std::cin.fail())
 {
 std::cout << "Incorrect input" << std::endl;
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 }
 }
}
#endif

Word.hpp

#ifndef WORD_HPP
#define WORD_HPP
#include <iostream>
#include <vector>
#include <algorithm>
class Word {
 public:
 Word();
 Word(std::string const & word);
 int length() {return m_len;};
 bool letterInWord(char letter);
 bool wordFound() {return std::find(std::begin(m_mask), std::end(m_mask), false) == std::end(m_mask);};
 void printLetterFound();
 
 friend std::ostream& operator<<(std::ostream & flow, Word const & word);
 
 private:
 std::string m_word;
 int m_len;
 std::vector<bool> m_mask; 
};
#endif

main.cpp

#include <iostream>
#include <fstream>
#include <ctime>
#include "Game.hpp"
#include "Settings.hpp"
#include "File.hpp"
int main()
{
 srand(time(NULL));
 bool playAgain {false};
 
 do {
 DIFFICULTY difficulty;
 LANGUAGE language;
 int numberOfLetters;
 Game::askUserSettings(difficulty, language, numberOfLetters);
 Game game(difficulty, language, numberOfLetters);
 
 game.initialisation();
 do {
 game.step();
 } while(!game.finished());
 playAgain = Game::askPlayAgain();
 } while(playAgain);
 return 0;
}

Game.cpp

#include <iostream>
#include <cassert>
#include "Game.hpp"
Game::Game(DIFFICULTY const difficulty, LANGUAGE const language, int const numberOfLetters) : m_count(0), 
 m_settings(difficulty, language, numberOfLetters),
 m_file(m_settings),
 m_word(m_file.getRandomWord()),
 m_limit(transformDifficultyInLimit(difficulty)) {}
void Game::askUserSettings(DIFFICULTY & difficulty, LANGUAGE & language, int & numberOfLetters)
{
 std::cout << "----------------------Welcome to the hangman game----------------------" << std::endl;
 std::cout << "Choose your difficulty : ";
 secureInput(difficulty, [](DIFFICULTY diff) -> bool {return diff == EASY || diff == MEDIUM || diff == HARD || diff == EXTREME;});
 std::cout << std::endl;
 std::cout << "Choose your language (for words) : ";
 secureInput(language, [](LANGUAGE lang) -> bool {return lang == FRENCH || lang == ENGLISH;});
 std::cout << std::endl;
 
 std::cout << "Choose word's number of letters : ";
 secureInput(numberOfLetters, [](int nbOfLet) -> bool {return nbOfLet >= 2 && nbOfLet <= 15;});
 std::cout << std::endl;
}
bool Game::askPlayAgain()
{
 std::string answer {""}; //easier to test predicate over string rather than bool
 bool playAgain {false};
 auto predicate = [](std::string answer) -> bool 
 { 
 return (checkStringBool(answer) == 1 || checkStringBool(answer) == 0); 
 };
 std::cout << "Do you want to play again : ";
 secureInput(answer, predicate);
 std::cout << std::endl << std::endl;
 playAgain = (checkStringBool(answer) == 1); //if string is a form of true (like true, yes, y...) then bool is true
 return playAgain;
}
void Game::initialisation()
{
 if (m_limit != -1) std::cout << "You have " << m_limit << " propositions to find the hidden word" << std::endl;
 std::cout << "Good Luck !!!" << std::endl << std::endl;
}
void Game::step()
{
 char letter {};
 std::cout << "The hidden word is : ";
 m_word.printLetterFound();
 std::cout << std::endl;
 std::string proposition = m_limit-m_count < 2 ? "proposition" : "propositions"; //to write singular or plural
 if (m_limit > 0) std::cout << m_limit-m_count << " remaining " << proposition << std::endl;
 std::cout << "Give a letter : ";
 secureInput(letter, isalpha);
 std::cout << std::endl;
 if (!m_word.letterInWord((char) toupper(letter))) ++m_count;
 std::cout << std::endl << std::endl;
}
bool Game::finished()
{
 if (m_count == m_limit)
 {
 std::cout << "You've lost, the word was : " << m_word << std::endl << std::endl;
 return true;
 }
 if (m_word.wordFound())
 {
 std::string proposition = m_count < 2 ? "proposition" : "propositions"; //to write singular or plural
 std::cout << "You've won, with " << m_count << " wrong " << proposition << ", the word was : " << m_word << std::endl << std::endl;
 return true;
 }
 return false;
}

File.cpp

#include <iostream>
#include <cstdlib>
#include <fstream>
#include <cassert>
#include "File.hpp"
File::File(LANGUAGE language, std::string const & filePath, int nbOfLetters) : m_language(language), 
 m_nbOfLetters(nbOfLetters), 
 m_filePath(filePath) 
{
 assert((m_language == FRENCH || m_language == ENGLISH) && "Language not correct");
}
File::File(Settings settings) : m_language(settings.getLanguage()), 
 m_nbOfLetters(settings.getNbOfLetters()),
 m_filePath(this->generateFilePath()) {}
int File::nbOfWordsInFile()
{
 int count {0};
 std::string word {""};
 std::ifstream file {m_filePath};
 if (file) 
 { 
 while(std::getline(file, word)) 
 { 
 ++count;
 }
 file.close();
 }
 else std::cout << "File doesn't exist !" << std::endl;
 return count;
}
std::string File::generateNameFile()
{
 return std::to_string(m_nbOfLetters) + "_letters.txt";
}
std::string File::generateLanguageFile()
{
 switch (m_language)
 {
 case FRENCH:
 return "french/";
 break;
 case ENGLISH:
 return "english/";
 break;
 default:
 return "french/";
 break;
 }
}
std::string File::generateFilePath()
{
 return "../../../words/" + this->generateLanguageFile() + this->generateNameFile();
}
std::string File::getWordFromFile(int position)
{
 assert(m_filePath != "" && "File path can't be \"\"");
 std::string word {""};
 std::ifstream file {m_filePath};
 if (file) 
 { 
 for (int i = 0; i < position; ++i)
 {
 std::getline(file, word);
 }
 file.close();
 }
 else std::cout << "File doesn't exist !" << std::endl;
 return word;
}
std::string File::getRandomWord()
{
 int randNumber {};
 std::string randomWord {};
 randNumber = randomNumber(this->nbOfWordsInFile());
 randomWord = this->getWordFromFile(randNumber);
 return randomWord;
}

Word.cpp

#include <iostream>
#include <cstdlib>
#include <fstream>
#include <cassert>
#include "File.hpp"
File::File(LANGUAGE language, std::string const & filePath, int nbOfLetters) : m_language(language), 
 m_nbOfLetters(nbOfLetters), 
 m_filePath(filePath) 
{
 assert((m_language == FRENCH || m_language == ENGLISH) && "Language not correct");
}
File::File(Settings settings) : m_language(settings.getLanguage()), 
 m_nbOfLetters(settings.getNbOfLetters()),
 m_filePath(this->generateFilePath()) {}
int File::nbOfWordsInFile()
{
 int count {0};
 std::string word {""};
 std::ifstream file {m_filePath};
 if (file) 
 { 
 while(std::getline(file, word)) 
 { 
 ++count;
 }
 file.close();
 }
 else std::cout << "File doesn't exist !" << std::endl;
 return count;
}
std::string File::generateNameFile()
{
 return std::to_string(m_nbOfLetters) + "_letters.txt";
}
std::string File::generateLanguageFile()
{
 switch (m_language)
 {
 case FRENCH:
 return "french/";
 break;
 case ENGLISH:
 return "english/";
 break;
 default:
 return "french/";
 break;
 }
}
std::string File::generateFilePath()
{
 return "../../../words/" + this->generateLanguageFile() + this->generateNameFile();
}
std::string File::getWordFromFile(int position)
{
 assert(m_filePath != "" && "File path can't be \"\"");
 std::string word {""};
 std::ifstream file {m_filePath};
 if (file) 
 { 
 for (int i = 0; i < position; ++i)
 {
 std::getline(file, word);
 }
 file.close();
 }
 else std::cout << "File doesn't exist !" << std::endl;
 return word;
}
std::string File::getRandomWord()
{
 int randNumber {};
 std::string randomWord {};
 randNumber = randomNumber(this->nbOfWordsInFile());
 randomWord = this->getWordFromFile(randNumber);
 return randomWord;
}

utils.cpp

#include <iostream>
#include <cstdlib>
#include <fstream>
#include <string>
#include "utils.hpp"
void stringToLower(std::string & s)
{
 std::transform(std::begin(s), std::end(s), std::begin(s), [](char letter) -> char {return std::tolower(letter);});
}
int checkStringBool (std::string s)
{ 
 stringToLower(s);
 if (s == "true" || s == "yes" || s == "y" || s == "1") return 1; 
 else if (s == "false" || s == "no" || s == "n" || s == "0") return 0;
 else return -1; //incorrect form of string
}
std::istream& operator>>(std::istream& flow, DIFFICULTY & difficulty)
{
 std::string diff;
 flow >> diff;
 stringToLower(diff);
 if (diff == "easy")
 difficulty = EASY;
 else if (diff == "medium")
 difficulty = MEDIUM;
 else if (diff == "hard")
 difficulty = HARD;
 else if (diff == "extreme")
 difficulty = EXTREME;
 return flow;
}
std::istream& operator>>(std::istream& flow, LANGUAGE & language)
{
 std::string lang;
 flow >> lang;
 stringToLower(lang);
 if (lang == "french" || lang == "fr")
 language = FRENCH;
 else if (lang == "english" || lang == "en")
 language = ENGLISH;
 
 return flow;
}
int randomNumber(int sup, int inf)
{
 return rand() % sup + inf;
}
int transformDifficultyInLimit(DIFFICULTY difficulty)
{
 switch (difficulty)
 {
 case EASY:
 return 20;
 break;
 case MEDIUM:
 return 15;
 break;
 case HARD:
 return 10;
 break;
 case EXTREME:
 return 5;
 break;
 default:
 return 15;
 }
}
//Useful to create files with words with different nb of letters from a big file that contains all words
// void createAllFilesTXT(LANGUAGE language, std::string const & bigFilePath)
// {
// std::string word {""};
// std::ifstream bigFile {bigFilePath};
// if (bigFile) 
// { 
// while(std::getline(bigFile, word)) 
// { 
// std::string nameFile = generateFilePath(language, word.size());
// std::ofstream file {nameFile, std::ios::app};
// file << word << '\n';
// }
// bigFile.close();
// }
// else std::cout << "File doesn't exist !" << std::endl;
// }

CmakeLists.txt

cmake_minimum_required(VERSION 3.10)
project(hangman)
set(SOURCES src/main.cpp src/Word.cpp src/Game.cpp src/Settings.cpp src/File.cpp src/utils.cpp)
add_executable(main ${SOURCES})
set_target_properties(main PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
target_include_directories(main PRIVATE include)

Answer 1

About the class organization

The organization of your code looks over-engineered to me. Let's start with File. The only purpose of this is to return a random word from a dictionary. However, if you construct it using Settings, it has to generate a path from the language and the number of letters you want. It somehow has to know where that file is relative to the current directory. But if you use the other form of the constructor, it takes a direct path to the file it will read, but also the language and number of letters. But the language and number of letters don't matter in that case, so why pass it to the constructor at all? And if this class is only used to generate only one word each time it is constructed, why make it a class at all? I would replace it with a plain function that returns a random word from a file:

std::string get_random_word(const std::string& filePath) {
 std::ifstream file(filePath);
 std::vector<std::string> words;
 std::string line;
 while (getline(file, line))
 words.push_back(std::move(line));
 if (!file.eof())
 throw std::runtime_error("Could not read word list");
 return words[randomNumber(words.size())];
}

Generating the filename can be done in a separate function:

std::string make_words_filepath(const Settings& settings) {
 ...
}

So that the constructor of Game can look like:

Game::Game(DIFFICULTY const difficulty, LANGUAGE const language, int const numberOfLetters) :
 ...,
 m_settings(difficulty, language, numberOfLetters),
 m_word(get_random_word(make_words_filepath(m_settings))),
 ... {}

Since these functions are only used by Game, you can consider making them private member functions of Game. Making a separate class File might be warranted if you have multiple files you want to handle at the same time, or if not just Game but other classes or functions would need to handle word list files as well. But since that is not the case, it is just needlessly complicating your code. See also the YAGNI principle.

I also think Word and Settings are not necessary. The functionality in those classes could be made private member functions of Game instead.

Naming things

File, Word, Game and Setting are all very generic names. In a larger project, that would be problematic. Make it a habit of making names more specific. For example, Game could be renamed to Hangman or HangmanGame.

As mentioned above, I don't think the other classes are necessary, but if you do want to keep them, then you can move these other classes into a namespace or into class HangmanGame.

ALL-CAPS names are usually reserved for macros and constants, and typically they are not used for types. So LANGUAGE and DIFFICULTY should be Language and Difficulty. Since the enum values themselves are like constants, I would say you can keep their names in all caps.

Use enum class

You get even better type safety by turning your enums into enum classes. For example, that way you cannot pass EASY to an int parameter.

Incorrect error checking

In utils.hpp, secureInput() checks if a value was read correctly, and if not will only quit if std::cin.eof() returns true. However, consider that an error can happen that's not reaching the end of file, and which cannot be recovered from: your code then goes into an infinite loop.

You have to separate I/O errors from errors parsing the input. You can check std::cin.bad() for non-recoverable errors. Alternatively, you can read in a whole line using getline(), as that doesn't do any parsing, so any error would be fatal, and then separately parse the line, for example by using std::stringstream.

Unnecessary code duplication

If you see yourself repeating almost the same code multiple times, find some way to avoid it. Consider secureInput(): you have three overloads for different numbers of predicates. Do you really need a version that handles two predicates? The caller can easily construct a new predicate that combines the two predicates it originally wanted to pass. You can also use a default argument for the predicate:

template <typename T, typename Predicate>
void secureInput(T & var, Predicate predicate = [](T&){return true;})
{
 ...
}

This also avoids any ambiguity: if you pass two predicates, should they both pass or just one? Alternatively, you could make a variadic template that handles any number of predicates (including none).

See also the DRY principle.

• Yann Depledt: Firsty thanks a lot for your complete review ! i will be refactoring my code thanks to your advices. But i have a few questions. i don't understand the part about secureInput(), my function is only use for users input so why i need to check if the program can go throught the file (maybe secureInput() was not clear sorry). Furthermore, about class organisation it's a bit messed up in my head. i agree that Settings and File are useless but for Word i am not sure. I understood that divide in max classes as possible is better if we want to add functions later.
• G. Sliepen: Even std::cin can fail such that eof() is not true. Consider that there could be I/O redirection so std::cin is actually pointing to a file, or a pipe, or that the terminal your program is running in is killed, or perhaps some other error is possible. As for adding functions later: if you are going to change the code anyway, you can start with no Word class and add it later. The big problem with splitting it up early is that it is easy to split it up wrong. When you really need it to be a class it's usually much more clear how to make it.