HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
youarewhowhereexerciseliveand

Problem

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 
#include 
#include 
using namespace std;

string a;
string b;

string enterName()
{
    while (true)
    {
        cout  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;
}

Solution

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 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.

Code Snippets

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

Context

StackExchange Code Review Q#63472, answer score: 16

Revisions (0)

No revisions yet.