How can I make a simple number guessing game more efficient?

Question

I have been programming a simple number guessing game and I am wondering if there is any way of making my code more efficient and cleaner. I have spent some time on it implementing error checking to make it as safe as I know how. I feel as if the way's that I have done this may be 'long winded' so if you know of a shorter way to do so then this would be much appreciated.

I do have comments, and I hope they make the program readable!

I am using Visual Studio 2010 - not sure if this changes much.

#include "stdafx.h"
#include <iostream>
#include <ctime>
#include <string>
#include <sstream>

// Include the standard namespace for easy use of cout/cin
using namespace std;

// Function Declarations.
string convertIntToString(int input);
bool isValidInput_Int(string input);
void cls();
void pause();
void printError(int ErrorNumber, bool ClearWindow);
void game(int difficulty);

// Main Function
int _tmain(int argc, _TCHAR* argv[])
{
    // Variables
    string input;
    int inputVal;
    bool gameIsRunning = true;

    // We want rand() to be as random as possible.
    srand( time(NULL) );

    // Run until the user wishes to quit.
    while(gameIsRunning)
    {
        cout << "Welcome to the number guessing game V1.0\n\nPlease select an option:\n";
        cout << "1- Easy\n2- Medium\n3- Hard\n4- Expert\n5- Exit\n>>";

        cin >> input;

        // Check if the data is valid according to our wishes.
        if(isValidInput_Int(input))
        {
            // Act correctly according to the input.

            // Store the integer we want.
            inputVal = atoi(input.c_str());

            // Make sure the input is a correct selection
            if(inputVal < 5 && inputVal > 0)
            {
                // Run the game.
                game(inputVal);
            }
            else if(inputVal == 5)
            {
                // Quit the application.
                exit(0);
            }
            else
            {
                printError(2, true);
            }
        }
        else
        {
            printError(1, true);
        }

    }

    // Leave the Application.
    return 0;
}

///--------------------------------------
/// Resource:
/// http://www.cplusplus.com/forum/beginner/7777/
/// Converts an integer to a string.
///--------------------------------------
string convertIntToString(int input)
{
   stringstream ss; //create a stringstream
   ss << input;     //add number to the stream
   return ss.str(); //return a string with the contents of the stream
}

///--------------------------------------
/// Checks if the input is of size 1 and
/// The values inside this are of type
/// Integer and nothing else.
///--------------------------------------
bool isValidInput_Int(string input)
{
    bool retVal = 0;
    try
    {
        if(atoi(input.c_str()))
            retVal = 1;
    }
    catch(exception)
    {
        retVal = 0;
    }
    return retVal;
}

///--------------------------------------
/// Clears the console window of text.
///--------------------------------------
void cls()
{
    system("cls");
    return;
}

///--------------------------------------
/// Pauses the console window.
///--------------------------------------
void pause()
{
    system("pause");
    return;
}

///--------------------------------------
/// Shows an error based on the input to
/// The function. If your error is not
/// Listed then put '0' in as a default
/// value. This returns a some-what
/// Generic message to the use.
///--------------------------------------
void printError(int ErrorNumber, bool ClearWindow)
{
    // Clear the console window
    if(ClearWindow)
        cls();

    switch(ErrorNumber)
    {
        case 1:
            cout << "---Error: Invalid Input---\n\n";
            break;
        case 2:
            cout << "---Error: Please enter a correct option---\n\n";
            break;
        default:
            cout << "---Error: Something went wrong---\n\n";
            break;
    }
    return;
}

///--------------------------------------
/// This is where the 'game' code is kept
///--------------------------------------
void game(int difficulty)
{
    // Variables
    string input;
    string message;
    int inputVal;
    // Generate our random number!
    int numberToGuess = rand() % ((difficulty * 2) * 10) + 1;

    cout << "Welcome!\n";
    cout << "You have 10 lives.\nThe number is between 1 and " << (difficulty * 2) * 10 << "\n";
    cout << "Start guessing!\n\n";

    // Loop until they are dead or they guess the number.
    for(int lives = 10;lives > 0; lives--)
    {
        cout << ">>";
        cin >> input;

        // Check if the data is valid according to our wishes.
        if(isValidInput_Int(input))
        {
            // Store the integer we want.
            inputVal = atoi(input.c_str());

            // Check if the guess was correct or not
            // If it isnt, give them some 'guidance'.
            if(inputVal == numberToGuess)
            {
                message = "Congratulations!\nYou Win!\n";
                lives -= 10;
            }

            if(lives != 1)
            {
                if(inputVal > numberToGuess)
                {
                    message = "Incorrect, try guessing lower!\n";
                }

                if(inputVal < numberToGuess)
                {
                    message = "Incorrect, try guessing higher!\n";
                }
            }
            else
            {
                message = "Incorrect, the number was: [" + convertIntToString(numberToGuess) + "]\n";
            }

            cout << message;
        }
        else
        {
            // Let them know that there input was invalid.
            printError(1, false);
            // We could 'punish' however, we will be nice.
            lives++;
        }
    }

    // Pause.
    pause();

    // Clear the screen before showing the menu again.
    cls();
    return;
}

Answer 1

Here some of my language-focused remarks, not touching the actual program design.

usings

// Include the standard namespace for easy use of cout/cin
using namespace std;

Very nice to list what you need by adding the comment. But instead of pulling all names you could make it more explicit

using std::cin;
using std::cout

In big files you may want to "sectionize" the using by from which #include they came from. (Edit, thanks to comment below) Best do not introduce using before other #includes though, you can group the using afterwards. E.g.:

#include <iostream>
#include <string>
#include <map>
#include <vector>
// for shorten code
using std::string;
using std::map;
using std::vector;
// from iostreeam
using std::cin;
using std::cout;

I do not do this for all names, only for the most frequent ones when it really saves space -- and most people do know anyway. Most std::-items I write with their namespace when I use them.

string I often apply a using to (lots of strings in function arguments), but map and vector I typically do not (better being explicit at the few location they are used). When I have to use lots of std::vector<thingy>::const_iterator somewhere I shorten the code with a local typedef or using.

void drawData(std::vector<thingy> &data) {
    typedef std::vector<thingy>::const_iterator tit;
    const tit end = data.end();
    for(tit it=data.begin(); it!=end; ++it) {
        *it.drawYourself();
    }
}

And with C++11 you do not even need that, you have auto and ranged-for to beautify your.

variable initialization

// Variables
string input;
int inputVal;
bool gameIsRunning = true;

It is ok not to initialize string, because it is a class/object.

But you should initialize int variables.

latest possible declaration

// Variables
string input;

...
while(...)
   ...
   cin >> input;

Your rule-of-thumb should be to declare a variable as late as possible which would mean

...
while(...)
   ...
   string input;
   cin >> input;

The exception are "tight loops" when the frequent initialization and destruction would cost to much. But actually, string is quite cheap and this is not a "tight loop", so I would recommend it.

Answer 2

On Comments

I do have comments, I hope they make the program readable!

Unfortunately, no. Your comments don’t help readability and only take up unnecessary space (which makes the program less readable).

The problem is that your comments convey useless information. For instance,

// Pause.
pause();

How is this comment helping?

There’s a commonly-quoted adage,

Code explains the how, comments explain the why

which actually covers most cases: use code to explain how a solution is implemented. Only use comments when necessary to explain why you do something.

In practice, this means that most code doesn’t need comments at all, good code should ideally be self-explanatory; if it isn’t, take that as a hint to rewrite it.

Here’s a positive example, however:

// We could 'punish' however, we will be nice.
lives++;

Now this comment is more helpful. But it could still be a bit clearer.

There’s one exception to this no-comments rule:

Do provide documentation for all functions via comments. But again, use the documentation to explain stuff that is not explained by the function name and signature.

For instance,

///--------------------------------------
/// Pauses the console window.
///--------------------------------------
void pause();

… not particularly helpful. What does “pauses the console window” actually mean? A more helpful description would be, “Halts execution until the user hits any hey”.

Your other function documentations are much better in this regard. But take care to explicitly describe all parameters, the return value, and all side effects.

Implementation

(In addition to what @towi already wrote.)

Input

Your input routine does three times the work:

1. It inputs a string
2. It tests whether this is a valid number, via isValidInput_Int.
3. It converts it to a number via atoi.

isValidInput_int is actually a lot of boilerplate code doing the same thing as just calling atoi. In fact, isValidInput_int doesn’t work at all because atoi cannot be used reliably to test whether a valid number was entered. It doesn’t throw an exception, it just silently returns 0. You have no way of knowing whether the user entered 0 or an invalid number.

Furthermore, isValidInput_int uses the bool type but you never actually use the bool constants true and false, instead you use 1 and 0. Do not do this. Unfortunately, C++ allows this but it’s a weakness of the type system (thanks to C).

Furthermore, atoi is redundant here anyway, just input the number directly. That is, instead of reading a string, do this:

int difficulty;
if (cin >> difficulty) {
    // Input was succesful, proceed.
}
else {
    printError(1, true);
}

Use meaningful variable names

(difficulty * 2) * 10 crops up several times. What does it mean? Put it into a meaningful variable to increase readability (range would be a meaningful name).

inputVal describes where the value comes from, but not what it signifies. How about nextGuess instead?

Also, you should settle on a single convention for names. At the moment you mix between camelCase and under_score, sometimes_WithCapital_Letters. Modern C++ code generally uses lowercase_words_separated_by_underscore but you’re free to use your own convention – the important thing is consistency.

Other stuff

You seem to always finish your functions with return; – that’s redundant. You only need return if you want to return a value, or if you want to exit the function prematurely. At the end of a function, you don’t need it.

Furthermore, main is a special case, return 0; is implied at the end of it, so this is also redundant (but all other functions that return values need explicit returns).

Finally, the logic flow in game isn’t entirely obvious. This could be cleared up. In particular, when do lives go up and down? It might help to simplify the input reading routine (see above) and to extract the guess into a separate function. In general, try to limit the level of nesting drastically. Four levels of nesting (here, a for and three ifs) should really be the exception. Your logic here could really be simplified, for instance by reversing the condition if (lives != 1), and exiting the loop early:

if(lives == 1)
{
    cout << "Incorrect, the number was: [" + convertIntToString(numberToGuess) + "]\n";
    break;
}

if(inputVal > numberToGuess)
{
    message = "Incorrect, try guessing lower!\n";
}

if(inputVal < numberToGuess)
{
    message = "Incorrect, try guessing higher!\n";
}

In my opinion, this still isn’t as readable as it could be because it takes up a lot of unnecessary vertical space. I find the following much more readable but be aware that this opinion isn’t shared by all.

if(lives == 1) {
    cout << "Incorrect, the number was: [" + convertIntToString(numberToGuess) + "]\n";
    break;
}

if(inputVal > numberToGuess)
    message = "Incorrect, try guessing lower!\n";

if(inputVal < numberToGuess)
    message = "Incorrect, try guessing higher!\n";

Some people will hate me for giving this advice but they probably wear their pants on their head. ;-)