patterncppMinor
Print the name of day
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
Make sure you have all required
The code uses
Don't use
There are two reasons not to use
Use
The
Use better naming
The vector named
Use string concatenation
The
Each of those is a separate call to
Isolate platform-specific code
If you must have
stdafx.h, consider wrapping it so that the code is portable:#ifdef WINDOWS
#include "stdafx.h"
#endifMake sure you have all required
#includesThe 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 practicalThe
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"
#endifvoid 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 forContext
StackExchange Code Review Q#161545, answer score: 7
Revisions (0)
No revisions yet.