5
\$\begingroup\$

I recreated the game Hangman and I'm decently happy with it, but like always it would be preferable if anyone knew how to improve my code:

main.cpp

#ifndef UNICODE
#define UNICODE
#endif

#include <iostream>
#include <conio.h>
#include <Windows.h>
#include <ctime>

#include "gameInfo.h"

// Function declares
std::wstring gen(gameInfo& info);
void clrscr();
void greet(gameInfo& info);
int ask(gameInfo& info, std::wstring& word, std::string& out);
void checklives(gameInfo& info);
void findindex(std::wstring& str, wchar_t character, std::vector<unsigned int>* vct);
void run(gameInfo& info);

// Main thread
int main() {

    srand((unsigned int)time(NULL));
    gameInfo info;

    greet(info);
    clrscr();

    // Runs the whole game  
    run(info);  
}

// Returns random word from word list
std::wstring gen(gameInfo& info) {
    return info.dictionary()[rand() % info.dictionary().size()];
}

/* Function to get input from user at start of app
   makes user choose if they want to play or quit */
void greet(gameInfo& info) {

    while (true) {
        int choice;
        std::wcout << L"1: Play" << std::endl;
        std::wcout << L"2: Quit" << std::endl;

        if ((std::cin >> choice) && (choice == 1 || choice == 2)) {
            if (choice == 1) {
                info.run(true);
                break;
            }
            else break;
        }
        else std::wcout << L"Failed to get input from user" << std::endl;
    }
}

/* Asks user for a character and if there is that
   character in the string then for each of that
   character push back an index and for each index 
   replace the blank character with the input character
   and in the end output the string */
int ask(gameInfo& info, std::wstring& word, std::wstring& out) {

    std::wcout << L"The word is " << word.size() << L" characters long." << std::endl;
    std::wcout << L"You have " << info.live() << L" lives." << std::endl;
    std::wcout << "Input a character: " << std::flush;

    wchar_t answer;

    std::vector<unsigned int> index = { };
    if (std::wcin >> answer) {

        for (int i = 0, n = word.size(); i < n; ++i) {
            if (out[i] == L' ')
                out[i] = L'_';
        }

        answer = std::tolower(answer);
        findindex(word, answer, &index);
        if (!index.size()) {
            info.live(info.live() - 1);
            std::wcout << out << std::endl;
            return 1; // Success
        }

        for (unsigned int i : index) {
            out[i] = answer;
        }


        if (out == word) {
            std::wcout << L"Correct! the word was " << word << "." << std::endl;
            info.run(false);
            return 1; // Success
        }

        std::wcout << out << std::endl;
    }
    else {
        std::wcout << L"Error getting user input" << std::endl;
        return 0; // Failure
    }

    return 1; // Returns success
}

// Shite but does the job
void clrscr() {
    for (int i = 0; i < 10; ++i)
        std::wcout << L"\n\n\n\n\n\n\n\n\n" << std::endl;
}

// Finds index of a word
void findindex(std::wstring& str, wchar_t character, std::vector<unsigned int>* vct) {

    for (int i = 0, n = str.size(); i < n; ++i)
        if (str[i] == character)
            vct->push_back(i);
}

// Checks, well, lives.
void checklives(gameInfo& info) {
    if (info.live() < 1) {
        info.run(false);
    }
}

/* Runs the whole game. For each game loop asks
   the user if they want to retry or quit the game */
void run(gameInfo& info) {

    while (true) {

        std::wstring word = gen(info);
        std::wstring out(word.size(), L' ');
        while (info.run()) {
            ask(info, word, out);
            checklives(info);

            Sleep(1000);

            clrscr();
        }

        info.live(5);

        unsigned int choice;        
        std::wcout << L"1: Retry" << std::endl;
        std::wcout << L"2: Quit" << std::endl;

        if ((std::cin >> choice) && (choice == 1 || choice == 2)) {
            if (choice == 1) {
                info.run(true);
                clrscr();
            }

            else break;
        }
        else {
            std::wcout << L"Couldn't process input. " << std::endl;
            clrscr();
        }
    }
}

gameInfo.header

#pragma once

#include <vector>
#include <string>

class gameInfo {

private:

    // Main variables
    bool bRun = false;
    unsigned int lives = 5;

    std::vector<std::wstring> vec = { 
        L"banana", L"apple", L"circus", L"baby", L"key",
        L"cooker", L"bottle", L"keyboard", L"head", L"book",
        L"robe", L"cup", L"mug", L"box", L"dog", L"horse",
        L"potato", L"desk", L"winter", L"life", L"death"
    };

public:

    // Getters
    std::vector<std::wstring> dictionary();
    bool run();
    unsigned int live();

    // Setters
    void run(bool opt);
    void live(unsigned int opt);

};

gameInfo.cpp

#include "gameInfo.h"

std::vector<std::wstring> gameInfo::dictionary() { return vec; }
bool gameInfo::run() { return bRun; }
unsigned int gameInfo::live() { return lives; }

void gameInfo::live(unsigned int opt) { lives = opt; }
void gameInfo::run(bool opt) { bRun = opt; }
\$\endgroup\$
  • \$\begingroup\$ Well, you code is not portable. Is there a reason not to stick to standard C++ features? \$\endgroup\$ – L. F. May 5 at 10:37
  • \$\begingroup\$ Sorry, but i really don't know what you mean by that... \$\endgroup\$ – Nadpher May 5 at 10:42
  • \$\begingroup\$ "Not portable" means: you code is good, but can only run on some platforms (in this case, Windows). Did you consider the possibility of writing platform-independent programs? \$\endgroup\$ – L. F. May 5 at 10:44
  • 3
    \$\begingroup\$ This question is good. Rather than second game you might want to say Refactored or Improved in the title. You should also provide a link to your first question in the body of the question. You might want to delete your answer to the first question since it is basically this question. \$\endgroup\$ – pacmaninbw May 5 at 11:38
  • 2
    \$\begingroup\$ @L.F. I'd wait a little while in case the question gets edited, but you provided a pretty good answer in your comments, you might want to make it into an answer. \$\endgroup\$ – pacmaninbw May 5 at 11:39
5
\$\begingroup\$

Common File Names There are 2 generally accepted extensions for C++ header files, they are .h and .hpp. A professional programmer will stick to the common extensions so that others can maintain his code.

Many editing tools will create these files for you, for instance in Visual Studio you can select Add from the solution explorer and then select class. Visual Studio will then generate class.h and class.cpp. Visual Studio will also create the constructor and destructor declarations in the header file and default bodies for the constructor and destructor in the cpp file.

Initializing Classes
This line in main() not only creates the gameInfo object in memory, it also calls the default constructor which initializes the variables in gameInfo:

    gameInfo info;

It is more common to see initialization of private variables within the constructor, rather than in the declaration of the class. If the body of the constructor is in the cpp file this prevents the recompile of all the files that include the header if the value of one of the variable needs to change. This becomes important as your programs grow and multiple classes are required. I've worked on projects where recompiling everything takes more than 30 minutes.

A default constructor for gameInfo might look something like this:

std::vector<std::wstring> initVec = {
    L"banana", L"apple", L"circus", L"baby", L"key",
    L"cooker", L"bottle", L"keyboard", L"head", L"book",
    L"robe", L"cup", L"mug", L"box", L"dog", L"horse",
    L"potato", L"desk", L"winter", L"life", L"death"
};

gameInfo::gameInfo()
    : bRun{false}, lives{5}
{
    vec = initVec;
}

and a parameterized constructor might look something like this:

gameInfo::gameInfo(bool run, unsigned int lifeCount)
    : bRun{run}, lives{lifeCount}
{
    vec = initVec;
}

Public Before Private
It might be better to put all the public methods before the private methods in the class definition. The public methods will be used by the consumers and having them at the top makes it easier to find.

It is possible to put the bodies of functions into the headers, this is necessary for template functions, but it is also useful for simple functions such as the getters and setters in gameInfo.

Using the items discussed above, here is an example how gameInfo might be declared in gameInfo.h:

#include <vector>
#include <string>

class gameInfo
{
public:
    gameInfo();
    ~gameInfo() = default;

    // Getters
    std::vector<std::wstring> dictionary() const { return vec; };
    bool run() const { return bRun; };
    unsigned int live() const { return lives; };

    // Setters
    void run(bool opt) { bRun = opt; };
    void live(unsigned int opt) { lives = opt; };

private:
    // Main variables
    bool bRun;
    unsigned int lives;

    std::vector<std::wstring> vec;
};

In the declaration above the constructor and destructor have been added, the private variables are declared but their initialization has been moved into the constructor. The bodies of the simpler functions have been moved from the CPP file to the class declaration and the public interfaces have been moved up.

Putting the keyword const before the body of the function when the function doesn't change any values can improve the performance of the program.

The keyword default in the declaration of the destructor indicates that the compiler should generate a default destructor for this class. This is useful when the class doesn't have resources such as files that need to be closed.

Complexity
Many of the functions in this program are simple, atomic and straight forward (nice work), but both the ask function and the run function in main.cpp might be candidates for simplification into multiple functions.

The program as a whole can be simplified by moving almost all of the functions into the gameInfo class. All the functions that receive info can then access the private members directly and don't require the setter and getter functions. The main() function could be simplified to :

int main() {

    srand((unsigned int)time(NULL));
    gameInfo info;

    info.greet();
    info.run();
}

The seeding of the random number generator could also be moved to the gameInfo constructor since that is only constructed once.

Most of the functions in gameInfo would then become private functions since they are only required by gameInfo and not other functions or classes.

Examples of simplified functions in gameInfo:

// Returns random word from word list
std::wstring gameInfo::gen()
{
    return vec[rand() % vec.size()];
}

// Checks, well, lives.
void gameInfo::checklives() {
    if (lives < 1) {
        bRun = false;
    }
}
\$\endgroup\$
1
\$\begingroup\$

I just spotted some random things not mentioned yet.

Make the wait portable

Consider using a modern portable way to replace

#include <windows.h>

Sleep(1000);

with

#include <chrono>
#include <thread>

std::this_thread::sleep_for(std::chrono::milliseconds{ 1000 });

This approach works on all platforms were c++11 is available.

Portable the second

Annother thing:

void clrscr();

This function comes from the ancient conio.h. It can be replaced with system calls to clean the screen

// somewhere in the program
#define WINDOWS 1

void console_clear_screen() {
  #ifdef WINDOWS
  system("cls");
  #endif
  #ifdef LINUX
  system("clear");
  #endif
}

consider not using unsigned int

Also you should consider if you really really need unsigend int. It can be source for errors. See: https://stackoverflow.com/questions/22587451/c-c-use-of-int-or-unsigned-int

use better random generator

This:

srand((unsigned int)time(NULL));

should be replaced with functions from . See: https://stackoverflow.com/questions/52869166/why-is-the-use-of-rand-considered-bad

\$\endgroup\$
  • \$\begingroup\$ clrscr(); is the old DOS clear-screen in conio.h. (another portability problem) \$\endgroup\$ – David C. Rankin May 11 at 4:38
  • \$\begingroup\$ a good point. But still he declares the function void clrscr(); himself. See top of the code. anyway i edited my post. \$\endgroup\$ – Sandro4912 May 11 at 7:41
  • \$\begingroup\$ Hah, correct you are, but I suspect it is void clrscr (void) { clrscr(); } :) \$\endgroup\$ – David C. Rankin May 11 at 7:42

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.