15
\$\begingroup\$

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 <fstream>
#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <string>
#include <time.h>
#include <vector>

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

void Clear_Screen(){
    cout << string(1000, '\n');
}

vector<string> Add_Lines(unsigned short int usiLength){
    vector<string> vecsLines_Vector;
    for(unsigned short int i = 0; i < usiLength; i++){
        vecsLines_Vector.push_back("_");
    }
    return vecsLines_Vector;
}

string Output_Guesses(vector<string> vecsVector, unsigned short int usiLimit){
    string sOutput;
    for(unsigned short int usiI = 0; usiI < usiLimit; usiI++){
        sOutput += vecsVector[usiI] + " ";
    }
    return sOutput;
}

int main(){

    while(true){

        cout << "\t" << " __   __  _______  __    _  _______  __   __  _______  __    _ " << endl
             << "\t" << "|  | |  ||   _   ||  |  | ||       ||  |_|  ||   _   ||  |  | |" << endl
             << "\t" << "|  |_|  ||  |_|  ||   |_| ||    ___||       ||  |_|  ||   |_| |" << endl
             << "\t" << "|       ||       ||       ||   | __ |       ||       ||       |" << endl
             << "\t" << "|       ||       ||  _    ||   ||  ||       ||       ||  _    |" << endl
             << "\t" << "|   _   ||   _   || | |   ||   |_| || ||_|| ||   _   || | |   |" << endl
             << "\t" << "|__| |__||__| |__||_|  |__||_______||_|   |_||__| |__||_|  |__|" << endl << endl

                             << "\t\t\t\t" << "---------------" << endl
                             << "\t\t\t\t" << "| 1. New Game |" << endl
                             << "\t\t\t\t" << "| 2. Exit     |" << endl
                             << "\t\t\t\t" << "---------------" << endl
                             << "\t\t\t\t" << "Selected : ";

            // Prevents from entering an invalid value
            unsigned short int usiMenu_Select = 0;
            do{
                cin >> usiMenu_Select;
            }while(usiMenu_Select < 1 || 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<string> 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<string> vecsGuesses = Add_Lines(kusiWord_Length);
            vector<string> vecsWrongs = Add_Lines(kusiTries);

            // Guessing game loop
            unsigned short int usiIncorrect = 0;
            unsigned short int usiCorrect = 0;
            while(usiIncorrect < kusiTries){

                cout << "\t\t\t" << "Correct : " << Output_Guesses(vecsGuesses, kusiWord_Length) << endl << endl <<
                "\t\t\t" << "  Wrong : " << Output_Guesses(vecsWrongs, kusiTries) << endl << endl <<
                "\t\t\t" << "  Guess : ";

                // Prevents from entering more than 1 character
                string cGuess;
                do{
                    cin >> cGuess;
                }while(cGuess.length() > 1);

                // Checks if letter guessed is part of the word
                bool bValid = false;
                for(unsigned short int i = 0; i <= kusiWord_Length; i++){
                    const string ksValid_Character = ksWord_Selected.substr(i,1);
                    if(cGuess == ksValid_Character){
                        vecsGuesses[i] = cGuess;
                        usiCorrect++;
                        bValid = true;
                    }
                }

                // Checks if answer is valid
                if(!bValid){
                    vecsWrongs[usiIncorrect] = cGuess;
                    usiIncorrect++;
                }

                // Output end game message
                if(usiCorrect == kusiWord_Length){
                    cout << "\t\t\t" << " _ _ _ _                 " << endl
                         << "\t\t\t" << "| | | |_|___ ___ ___ ___ " << endl
                         << "\t\t\t" << "| | | | |   |   | -_|  _|" << endl
                         << "\t\t\t" << "|_____|_|_|_|_|_|___|_|  " << endl
                         << "\t\t\t" << "Enter any keys to continue..";
                         cin >> cGuess;
                         usiIncorrect = kusiTries;
                }else if(usiIncorrect == kusiTries){
                    cout << "\t\t\t" << " __                    " << endl
                         << "\t\t\t" << "|  |   ___ ___ ___ ___ " << endl
                         << "\t\t\t" << "|  |__| . |_ -| -_|  _|" << endl
                         << "\t\t\t" << "|_____|___|___|___|_|  " << endl
                         << "\t\t\t" << "Enter any keys to continue..";
                         cin >> cGuess;
                         usiIncorrect = kusiTries;
                }
                Clear_Screen();
            }
    }
}
\$\endgroup\$
14
\$\begingroup\$
  • 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 <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>
    
  • Use <ctime> instead of <time.h>. 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<std::string> 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<unsigned int>(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.

\$\endgroup\$
  • \$\begingroup\$ While i agree with most of what you've said and have started using all of your tips, I'm not quite sure about the whole "Put chunk of codes into functions to save space". Mainly because, if i don't use this function twice, it's gonna be a waste of memory plus, it doesn't actually help because I'm just moving the chunk of code outside of main(). So my file is still gonna be a mess. \$\endgroup\$ – user902934 Aug 27 '13 at 23:31
  • \$\begingroup\$ Even if it still looks like a mess, at least you can separate the concerns. For instance: the block following the comment Checks if letter guessed is part of the word. If that were in a function, you would just do a function call in its place in main(). If you then want to go back and improve it or add to it, you know right where it is. As for memory as a whole, creating functions doesn't have a significant effect on it. And I've already mentioned readability. It's not all about the space; functions won't always save that much. It's much about organization. \$\endgroup\$ – Jamal Aug 27 '13 at 23:48

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.