Hello lazorgod,
Overall the program works. I do have some suggestions and explanations that should help.
You may not be use to it yer or even covered its use, but I follow "iostream" with the header "iomanip". As the name suggests they are manipulators to the output streams. You can read up on it at:
http://www.cplusplus.com/reference/iomanip/ and
http://www.cplusplus.com/reference/ios/ios_base/fmtflags/ I will show you its use later.
For the line
using namespace std; // <--- Best not to use.
The comment says it best.
Lines 18 - 29 as Cheddar99 has said you need to make these variables constants. In contrast to his note everything I have read here and elsewhere says the a variable defined as a constant should be in capital letters. Whether you put two or more together as one word or use the underscore, as a space, to separate words is your choice, but choose one method and be consistent.
If your compiler is up to the C++11 standards the newer form for numeric variables is
constexpr
otherwise
const
will do. From C++11 on the {}s or uniform initializer is easiest to use. An empty set {} will initialize to the correct form of zero based on the type of variable. Or you could put a number inside the {}. This also works with std::strings.
Example:
1 2 3
|
constexpr int JAN{ 31 };
constexpr int FEB{ 28 };
constexpr int MARCH{ 31 };
| |
Something that would make the coding easier is:
1 2
|
const std::string MONTHS[]{ "", "January", "February", "March" };
constexpr int MONTH_LENGTHS[]{ 0, 31, 28, 31 };
| |
Note: the first element of each array is empty, or call it more a place holder, to allow January to be in element "1" of the array. This way when you use "numMonth", This works, but ("monthNum") is a better description of what it is used for and easier to read, can be used as the subscript to access the correct month. This is a case of making the array one larger and not using element zero.
By using the array "MONTHS" you could eliminate the need for the function "MonthToString" and just say
outFile << MONTHS[month] << " " << day << ...
in "main". Or if you need the function you could shorten it to just:
1 2 3 4
|
std::string MonthToString(int numMonth)
{
return MONTHS[numMonth];
}
| |
Much easier than all those is/else if statements or even a switch.
Inside "main" I like to initialize all the variables to be save in the knowledge that they have a known value before they are used. In your program this is not necessary as these variables are given a value before they are used.
For your file streams. Sometimes it is easier to give each file stream its own variable name like:
1 2 3
|
std::ifstream inFile("Days.txt");
std::ofstream outFileErr("error.txt");
std::ofstream outFileRes("results.txt");
| |
In this example you not only create the file stream and variable name "inFile" what is in the ()s will open the file at the time the stream is created. What is in the ()s can either be the file name in double quotes or a variable name like your "fileName".
Your if statement
if (!inFile)
is good, but I would add one last line:
return 1; // exit(1); // if not in "main".
Since zero denotes a normal return from the program any number greater than zero means there is a problem. Should you have more than one if statement like this using different numbers for the return statements can help to track down what went wrong. The reason for the return statement is that there is no point in continuing with the program until you fix the problem.
In contrast opening a file stream for output is less likely to have a problem because if the file name does not exist it will create it, but there is still a possibility that something could go wrong and it is a good idea to check the output file streams after they are opened.
In your original code you have:
1 2 3 4 5 6
|
if (!din)
{
}
else
{
}
| |
This may work, but, and I am sorry I can not think of a nice way to say this, putting what is in "main" inside an if block or else block is not the best way to write the code. And it is kind of tacky. The point of the return statement in the if block is to exit the program and fix the problem. Should you bypass the if statement then you just continue on with the program.
What I can say is that an if statement does not always need an else statement or even an else if statement. The if statement can work on its own do what it has to do and then continue with the program.
After running the program a couple of times I realized that the "PrintHeading" function should be called before the while loop and not inside the loop where it was printing to many headings.
In your while condition
while (din)
and it is much better than
while (!din.eof())
which does not work the way you think it does. By the time the while condition figures you have reached eof you will have processed your last read a second time.
To read a file of unknown length this is most often used:
while (inFile >> month >> day >> year)
. This way when it tries to read past end of file or either of the other two variables has a problem the file stream will set the eof, fail or bad bit making the condition false and the while loop will end.
Inside the while loop you have problems with your file stream. Opening the file stream "dout" inside the while loop works the first time through the loop, but causes the stream to fail when you try to open "dout" when it is already open. After that you are nor able to write to the file any more. The next problem is when you try to open "dout" for "errost.txt". Since "dout" is already open for "results.txt" it will cause the stream "dout" to fail and nothing will be written to either file.
One solution is to open the stream for append and after writing to it close the stream so it can be used again. This also applies when you open the stream for "errors.txt". The problem is that this will cereate overhead that could slow the program down.
I would say the better choice is to open three file streams before the while loop and then close them after the while loop. This way you use the output files as you need them and close them when you are finished reading the input file.
Using the array I showed you the "CalcDays" function can be reduced to a for loop:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
|
int CalcDays(int numMonth, int date)
{
int dayOfYear{};
if (numMonth == 1)
{
return date; //for January, the day of the year is just the date entered
}
for (int lc = 1; lc < numMonth; lc++)
{
dayOfYear += MONTH_LENGTHS[lc];
}
return (dayOfYear + date);
}
| |
End part 1.