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

Print the name of day

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

Problem

How can I improve this code? It takes a number in the range of [1,7] and prints its corresponding day.

#include "stdafx.h"
#include      // std::cout
#include        // std::vector
#include 

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

int main()
{
    vector name_day = { "Saturday" ,"Sunday" , "Monday" , "Tuesday" , "Wednesday" , "Thursday" , "Friday" };
    int n ;

    cout > n;

        if (!cin) //If input is wrong.
        {
            // reset failbit
            cin.clear();
            cin.ignore(std::numeric_limits::max(), '\n');
        }

        //If the condition is not satisfied .

        else
        {
            for (int i = 0; i <= 6; i++)
            {
                if (n-1 == i)
                {
                    cout << name_day[i] << "\n" << "-----------------------" << "\n";;
                    flag = false;
                }

            }//end of for 
        }

        if (flag) { cout << "Bad Entery !." << "\n" << "-----------------------" << "\n"; }

    }//end of while 

    system("pause");
}

Solution

Here are some things that may help you improve your program.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif


Make sure you have all required #includes

The code uses std::numeric_limits but doesn't #include . A program may compile without the explicit #include because some other header might happen to include it, but that is not guaranteed by the standard. To compile reliably, the documented #include files should be used.

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). 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 PAUSE or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions pause() 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++. For example:

void pause() {
    getchar();
}


Use const where practical

The name_day vector is not and should not be modified, so it should be declared const. Better yet, it could be declared static const.

Use better naming

The vector named name_day is well named because it is descriptive of the contents. However, flag is not a good name because it doesn't suggest the meaning of the variable in the context of the program.

Use string concatenation

The main routine includes this very long line:

std::cout << "This program takes a number in the range of" << "\n" << "[1 , 7] and prints its corresponding day." << "\n" << "............................................................." << "\n";


Each of those is a separate call to operator and #include are no longer needed.

Code Snippets

#ifdef WINDOWS
#include "stdafx.h"
#endif
void pause() {
    getchar();
}
std::cout << "This program takes a number in the range of" << "\n" << "[1 , 7] and prints its corresponding day." << "\n" << "............................................................." << "\n";
std::cout << "This program takes a number in the range of\n" 
             "[1 , 7] and prints its corresponding day.\n"
             ".............................................................\n";
for (int i = 0; i <= 6; i++)
{
    if (n-1 == i)
    {
        std::cout << name_day[i] << "\n" << "-----------------------" << "\n";;
        flag = false;
    }

}//end of for

Context

StackExchange Code Review Q#161545, answer score: 7

Revisions (0)

No revisions yet.