"Who are you, and where do you live?" — a Q&A exercise

Question

This is my first attempt at a programme. Is it good form? Does it follow best practice? Or am I completely off the mark?

#include "stdafx.h"
#include <iostream>
#include <string>
#include <Windows.h>
using namespace std;

string a;
string b;

string enterName()
{
    while (true)
    {
        cout << "what is your name?: ";
        getline(cin, a);
        if (a == "")
            cout << "Let's try that again." << endl;
        if (a != "")
            return a;
    }
}

string enterAge()
{
    while (true)
    {
       cout << "What is your age " << a << "?: ";
        getline(cin, b);
        if (b == "")
            cout << "You did not answer the question." << endl;
        if (b != "")
            return b;
    }
}

int verifyAge()
{
    while (true)
    {
        int x = atoi(enterAge().c_str());
        if (x > 0 && x < 95)
            return x;

        cout << "That is not possible, put your real age please." << endl;
    }
}

string address()
{
    while (true)
    {
        cout << "Where do you live " << a << "?: ";
        string c;
        getline(cin, c);
        if (c == "")
            cout << "You must live somewhere." << endl;
        if (c != "")
            return c;
    }
}

int _tmain(int argc, _TCHAR* argv[])
{
    system("Color 1A");
    string a = enterName();
    cout << endl;
    int y = verifyAge();
    cout << endl;
    string c = address();
    cout << endl;
    system("cls");
    cout << endl << endl << endl;
    cout << "So your name is " << a << " you are " << y << " years old" 
        << " and you live in " << c << endl << endl << endl;
    cout << "Press any key to exit";
    cin.get();
    return 0;
}

Answer 1

I have found a couple of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Avoid the use of global variables

I see that a and b are declared as global variables rather than as local variables. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable. It's a bit strange because you have done that with variable c in your _tmain function.

Use meaningful variable names

The variable names a, b, and c are not at all descriptive. Better names might be name, age and address. Doing so makes your code easier to read, understand and maintain.

Don't use system("cls")

There are two reasons not to use system("cls") or system("Color 1A"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or Color, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a separate functions cls() and color() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++.

General portability

This code could be made portable if, in addition to the changes in the previous point, you omit the Windows-only include files #include "stdafx.h" and #include <Windows.h> and use the standard int main() rather than the Windows-only _tmain. See this stackoverflow question for a longer explanation.

Fix your formatting

There are abundant examples here of C++ code that is well formatted. This code has peculiar indentation that makes it difficult to tell when a function begins and ends. Fixing that would help.

Make it clear when loops end

The code currently uses while (true) in a number of places, but it doesn't really loop infinitely. It's better, generally, to have the loop exit condition explicitly stated. For example we could rewrite your enterName() function:

string enterName()
{
    do
    {
        cout << "what is your name?: ";
        getline(cin, a);
        if (a == "")
            cout << "Let's try that again." << endl;
    } while (a == "");
    return a;
}

Variable range checking

Your verifyAge() routine seems to think that it's impossible for people to be 95 year old or older, but I personally know people that old who still use a computer. A number of 130 might be a safer bet to use as an upper range.

In all, the program actually functions as intended, so you are indeed on your way.

Answer 2

The first thing I notice when looking at your code is that it looks rather messy. This can easily be fixed by giving your code proper indentations. I'm not sure if the indentation is like this because you formatted it wrong on the website, but indenting your code neatly is one of the first steps to good code readability. Here's an example on how to keep it neat:

string enterName()
{
    while (true)
    {
        cout << "what is your name?: ";
        getline(cin, a);

        if (a == "")
            cout << "Let's try that again." << endl;
        if (a != "")
            return a;
    }
}

Another thing that helps improve code readability is splitting up code in blocks. In my example above you can see that I separated the reading/creating of the variable from the actions on the variable.

Another thing that could help improve readability is, instead of creating two if-statements, creating an if-else statement. So instead of:

if (a == "")
    cout << "Let's try that again." << endl;
if (a != "")
    return a;

Do this:

if (a == "")
    cout << "Let's try that again." << endl;
else
    return a;

This makes it more clear that if the variable is not empty it should be returned. Be aware that you shouldn't always use if-else statements of course, only when they're needed.

Another thing that you should definitely work on is naming your variables. Variables such as a don't tell other users, or yourself much. Imagine that you would use this variable a hundred lines down. Would you still remember what a is at that point? Or imagine someone else looking at your code, they won't know what all those variable names are. Another reason to do it is if you come back to the code after a year, because you need to add/fix/remove something. Would you still know what every variable does if they're named a b and c? Probably not, that's why I'd recommend using descriptive names for your variables. So instead of using a use name for example.

You'll also want to look at for your loop. At the moment it's not immediatly clear to other users when the loop ends. You can easily indicate this with a comment, but more preferably you should try to make the loop itself as clear as possible. See Edward's rewritten loop for a good example.