5
\$\begingroup\$

As of now this game only consists of:

  • Splash Screen
  • MainMenu
  • SubMenus

Did I apply bad practices, or did I do something well? Is there a better way to do it?

#include <iostream>
#include <fstream>///filestream lib-- needed for file i/o
#include <windows.h>///Needed for alt clear screen function
#include <iomanip>///setw-- for indentation on title screen or headers in game
#include <cstdlib>
#include <limits>///used for char -> int input error
#include <conio.h>/*Used for pause screen--Windows Specific-- MUST FIND PORTABLE SOLUTION, possibly ifdef*/
//#include <cctype>
#include <string>
#include <ctime>///used for initializer for now

/**********************************************/
///used to clear screen cross platform
///#ifdef _WIN32 //checks to see if windows OS
///#define CLEAR "cls"
///#else //In any other OS
///#define CLEAR "clear"
///#endif
/*********************************************/
int inputError();///custom error message at top of screen
void Clear();///Safer way to clear screen non system()
void SplashScreen();///Production logo
void LoadTextFile(std::string);
void Initialize(int, char**);///Loading game
void MainMenu();
void Help();
void Game();
/********************************************/
int main(int arg, char** argv)
{
    SplashScreen();///display Splash screen
    Initialize( arg, argv);///Pretend to be loading game
    Clear();///clear Splash and other text

    MainMenu();///Pretty Much the start of the main Game loop
}
/***********************************************************************/
void Clear()
{
    COORD topLeft  = { 0, 0 };
    HANDLE console = GetStdHandle(STD_OUTPUT_HANDLE);///get a handle to the console
    CONSOLE_SCREEN_BUFFER_INFO screen;
    DWORD written;///num of characters written

    GetConsoleScreenBufferInfo(console, &screen);

    FillConsoleOutputCharacterA(
        console, ' ', screen.dwSize.X * screen.dwSize.Y, topLeft, &written
    );
    ///Overwrite console with whitespace
    FillConsoleOutputAttribute(
        console, FOREGROUND_GREEN | FOREGROUND_RED | FOREGROUND_BLUE,
        screen.dwSize.X * screen.dwSize.Y, topLeft, &written
    );
    SetConsoleCursorPosition(console, topLeft);///reset cursor to topleft pos
}
/***********************************************************************/
int inputError()
{
    HANDLE hStdout = GetStdHandle(STD_OUTPUT_HANDLE);
    if (hStdout == INVALID_HANDLE_VALUE)
    {
        std::cout << "Error while getting input handle" << std::endl;
        return 1;
    }
    ///sets the color to intense red on blue background
    SetConsoleTextAttribute(hStdout, FOREGROUND_RED | FOREGROUND_INTENSITY);
    std::cout <<"Previous selection was invalid, try another from the list: " << std::endl;

    ///reverting back to the normal color
    SetConsoleTextAttribute(hStdout, FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE);
}
/***********************************************************************/
void SplashScreen()
{
    std::string Lines = "";///NULL string
    std::ifstream Splash("TextFiles\\splash.txt");///ifstream used to read ascii art from txt

    std::cout <<"Presented by...\n" << std::endl;
    if(Splash.is_open())
    {
        while(!Splash.eof())///checks to see if End Of File... Loops unil end
        {
            std::string tempString;
            std::getline(Splash, tempString);///Takes text from document and stores in tempString
            tempString += "\n";///adds Newline

            Lines += tempString;///
        }
        std::cout << Lines << std::endl << std::endl;
    }
    else
    {
        std::cout <<"ERROR file could not be loaded or does not exist!";
    }
}
/************************************************************************/
void LoadTextFile(std::string FileName)
{
    std::string Lines = "";///NULL string
    std::ifstream myFile(FileName.c_str());///ifstream used to read from txt

    if(myFile.is_open())
    {
        while(!myFile.eof())///checks to see if End Of File... Loops unil end
        {
            std::string tempString;
            std::getline(myFile, tempString);///Takes text from document and stores in tempString
            tempString += "\n";///adds Newline

            Lines += tempString;///
        }
        std::cout << Lines << std::endl << std::endl;
    }
    else
    {
        std::cout <<"ERROR file could not be loaded or does not exist!";
    }
}
/************************************************************************/
void Initialize( int argc, char** argv )
{
    std::string message = "LOADING GAME...";
    std::cout << message << std::flush;

    /// spin some cycles to pretend that the program is loading resources, etc.
    time_t start = time( &start );
    while (difftime( time( NULL ), start ) < 6.5 /**seconds*/){}

    std::cout << "\r" << std::string( message.length(), ' ' ) << "\r" << std::flush;
}
/***********************************************************************/
void MainMenu()
{
    int Choice;

    std::cout << std::setw(50) <<"Welcome To MY RPG!!!" << std::endl;
    std::cout << std::setw(73) <<"Enjoy! Your suggestions and constructive criticism are encouraged!" << std::endl;

    std::cout <<"\n\n\n\n\n\n\n\nPlease Select an Option: " << std::endl;
    std::cout <<"1: New Game \n2: Load Game(In Development) \n3: Help \n4: Quit\n" << std::endl;
    std::cin >> Choice;

    /**Fixes infinite loop cause by char input instead of numeric!!!*/
    while(std::cin.fail())
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
        Clear();
        inputError();
        MainMenu();
        //std::cout << "Bad entry.  Enter a NUMBER: ";
        //std::cin >> Choice;
    }

    switch(Choice)
    {
    case 1:
        break;
    case 2:
        break;
    case 3:
        Clear();
        Help();
        break;
    case 4:
        std::exit(0);
        break;
    default:
        Clear();
        inputError();
        MainMenu();
    }
}
/***********************************************************************/
void Help()
{
    int Choice;
    std::cout << std::setw(53) <<"Looks like you need help!" << std::endl;
    std::cout << std::setw(57) <<"Well, you came to the right place!" << std:: endl;

    std::cout <<"\n\n\n\n\n\n\n\nPlease Select an Option: " << std::endl;
    std::cout <<"1: Game Synopsis \n2: In-Game Command List \n3: Credits \n4: Back\n" << std::endl;
    std::cin >> Choice;

    while(std::cin.fail())
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
        Clear();
        inputError();
        Help();
    }

    switch(Choice)
    {
    case 1:
        break;
    case 2:
        Clear();
        LoadTextFile("TextFiles\\CommandList.txt");
        std::cout <<"Press any key to return to \"Help\" Screen: ";
        getch();
        Clear();
        Help();
        break;
    case 3:
        Clear();
        LoadTextFile("TextFiles\\Credits.txt");
        getch();
        Clear();
        Help();
        break;
    case 4:
        Clear();
        MainMenu();
        break;
    default:
        Clear();
        inputError();
        Help();
    }
}
\$\endgroup\$
4
\$\begingroup\$

Adding to Loki's answer:

  • It would be nice to turn those loose functions into one or more classes. Currently your are not using global variables, which is nice, but excessively structured/procedural code like this can easily lead to nasty global state that is hard to maintain and hard to debug. I suggest starting the migration to OOP early. Move the code to a Game class and start from there, breaking it further into other classes to achieve the Single Responsibility goal.

  • When you declare function prototypes, keep the parameter names. This is a way of self-documenting the code, so you don't need to comment trivial stuff.

  • Those large switch statements are crying to be refactored into functions. Instead of writing something like this:

       case 2:
           Clear();
           LoadTextFile("TextFiles\\CommandList.txt");
           std::cout <<"Press any key to return to \"Help\" Screen: ";
           getch();
           Clear();
           Help();
           break;
    

    Turn all that into a helper function:

    case OptInGameCommands :
        displayInGameCommandList();
        break;
    

    Also, the magic numbers, 1, 2, 3, etc would arguably be better as constants that describe the action that is performed for a given input form the player. E.g.:

    enum MenuOptions
    {
        OptGameSynopsis   = 1,
        OptInGameCommands = 2,
        // etc...
    };
    
  • Use more constants to replace comments. Good code with no comments in better than bad code with lots of comments. Instead of doing things like this:

       time_t start = time( &start );
       while (difftime( time( NULL ), start ) < 6.5 /**seconds*/){}
    

    Use a constant for that time amount:

    const double secondsToSpin = 6.5;
    time_t start = time( &start );
    while (difftime( time( NULL ), start ) < secondsToSpin) { }
    
  • A final word on naming: PascalCase names are popular on Windows programming. I personally think this naming is fine for functions and types, though I don't think it is a good idea also applying it to variables names, such as Line and Splash in your code. I recommend using only camelCase or snake_case for variables to differentiate them from class types and function names. And of course, whichever convention you choose, be consistent with it within the project, that's the important thing, rather then which one to choose.

\$\endgroup\$
  • 1
    \$\begingroup\$ Dude you rock! This was really helpful, I have been reading up on OOP for C++ haven't yet got the hang of it. That's the main reason I've stuck to functions and procedural code. I'll be working on converting this to OOP and using Constants/enums etc. as soon as I have corrected the mistakes I've made with this so far. I'll be taking into account everything you and Loki have said. I appreciate it =) Will update the code in the next day or two, as I won't be at my house today and most of tomorrow. \$\endgroup\$ – TDubs95 Dec 21 '14 at 21:58
  • \$\begingroup\$ @xChubz, that's great! But post a new question with the revised code instead. Take a look at this discussion. \$\endgroup\$ – glampert Dec 21 '14 at 23:01
4
\$\begingroup\$

This is wrong in every language:

    while(!myFile.eof())///checks to see if End Of File... Loops unil end
    {

This is because the last successful read read "up to" but not past the EOF. Thus there is no data left but the EOF flag is not set. So condition will allow the loop to be entered evn though the next read is going to fail.

So this line:

        std::getline(myFile, tempString);
        tempString += "\n";
        Lines += tempString;

Will fail (after all the other lines have been read) and you are not checking for it. Thus Lines has an extra '\n' on the end at the very least. If you had written:

        if (std::getline(myFile, tempString))
        {
            tempString += "\n";
            Lines += tempString;
        }

Then Line would have at least bee correct.

But the better way to write this loop is:

    std::string tempString;
    while(std::getline(myFile, tempString))
    {
        Lines += (tempString + '\n')
    }
    std::cout << Lines << std::endl << std::endl;

Your comments are terrible:

SplashScreen();///display Splash screen

I can guess from the name of the function that the splashScreen is going to be displayed. I don't need the extra clutter of this comment. Comments should be reserved for telling my why something is happening. I can easily read in the code how it is happening (unless there is some big magic formula you may want to explain what that is doing).

Comments that repeat what the code is saying are actually WORSE than no comments. This is because over time the comments will fall out of step with the code. You need to take extra care to maintain the comments. Comments that are out of sync with the code are a real problem as the maintainer will come along and see that the code and comments do not match and will have to spend time trying to work out which are correct before fixing one or the other.

Prefer '\n' over std::endl

std::cout <<"Previous selection was invalid, try another from the list: " << std::endl;

// Just write
std::cout <<"Previous selection was invalid, try another from the list: \n";

The difference is an extra flush. The flush will not help you but it can definitely make the performance of your code much worse.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the info, really does help. My post was edited by someone else probably a moderator or something, I had said that I used all the comments for me. I didn't plan on posting the code it was me trying to make sure I knew what I was putting sorry. So yes the comments are terrible hahaha. Thanks for some brutally honest feedback I'll make the changes and neat it up as soon as I get home. I AM A NOOB so all things are helpful. \$\endgroup\$ – TDubs95 Dec 21 '14 at 16:53

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.