Algorithm that prints day, when submitted date

Question

I would love to know your opinion about this algorithm that we wrote in class. I was thinking about how I can optimize it (in terms of size of code/complexity) without using library functions (besides <iostream> for reading and writing).

Assignment (translated):

Gregorian calendar rule about leap years was introduced on (Friday) 15 of October in 1582. Write a program that when given a date in format “dd.MM.yyyy", will display the name of that day. You need to define and use enum type Weekday {Monday, Tuesday, …, Sunday}.

Here is my code:

   #include <iostream>


bool isLeapYear(int year){
    if(year % 4 == 0 && year % 100 != 0){
        return true;
    } else if (year % 400 == 0){
        return true;
    } else {
        return false;
    }
}
enum Weeknday {Monday, Tuesday, Wednesday, Thursday, Friday, Saturday, Sunday};
int main() {

    // Get date
    int day,mounth,year;
    scanf("%d.%d.%d",&day,&mounth,&year);

    // Count days
    int days = 0;
    for(int i = 1582; i <= year; i++){
        if(i != year) {
            if(isLeapYear(i)){
                days += 366;
            } else {
                days += 365;
            }
        } else {
            for(int j = 1; j <= mounth; j++){
                if(j != mounth){
                    if(j == 1 || j == 3 || j == 5 || j == 7 || j == 8 || j == 10 || j == 12){
                        days += 31;
                    } else if (j == 4 || j == 6 || j == 9 || j == 11){
                        days += 30;
                    } else if (j == 2){
                        if(isLeapYear(i)){
                            days += 29;
                        } else {
                            days += 28;
                        }
                    }
                } else {
                    days += day;
                }
            }
        }
    }



    int a = (days-3)%7;
    Weeknday enum_day;
    switch(a) {
        case 1:
            enum_day = Monday;
            std::cout << "Monday";
            break;
        case 2:
            enum_day = Tuesday;
            std::cout << "Tuesday";
            break;
        case 3:
            enum_day = Wednesday;
            std::cout << "Wednesday";
            break;
        case 4:
            enum_day = Thursday;
            std::cout << "Thursday";
            break;
        case 5:
            enum_day = Friday;
            std::cout << "Friday";
            break;
        case 6:
            enum_day = Saturday;
            std::cout << "Saturday";
            break;
        case 7:
            enum_day = Sunday;
            std::cout << "Sunday";
            break;

    }
    std::cout << enum_day;


}

Answer 1

Almost everything, including input and output, is in the main() function. That makes it hard to create automatic tests for the code. I recommend creating a function that can be tested without any I/O:

int weekday(int year, int month, int day)

Then main() can do the reading of input and writing of output.

---

We need to include <cstdio> to define std::scanf. The fact that you could build this on your platform without that include (and using plain scanf()) doesn't mean that it will build elsewhere, as that's due to optional choices made by your compiler that are not mandated by standard C++.

---

When we use std::scanf(), we absolutely must check how many values it converted, otherwise we'll be working with unitialised variables. Here's a suggestion:

    // Get date
    int day, month, year;
    if (std::scanf("%d.%d.%d", &day, &mounth, &year) != 3) {
        std::cerr << "Input must be in 'day.month.year' format.\n";
        return EXIT_FAILURE;
    }

(We need to include <cstdlib> to define EXIT_FAILURE).

---

In the switch to print the day name, we have case 7 which can never be reached. I think that was intended to be case 0.

We could use an array of day names instead of the switch.

---

If we define Weekday enum to begin with Sunday (which would then be equal to zero, we can convert our remainder of days directly to Weekday (int and enum types are convertible). If we're not allowed to change the order, we could change the arithmetic to reduce the value by 1.

---

Always produce complete lines of output. Each line should end with '\n'.

---

Spelling: it doesn't matter in a small program, but in larger programs (especially when working as part of a team) spelling errors can make searching for code difficult. Always try to use correct spellings for words such as "month" and "weekday".

---

The algorithm can be improved. It's possible to work out how many days to add for a range of years without looping over each one individually (and note that we only care about the number of days modulo 7, so we only need to count 1 or 2 days for each year).

Sticking with the iterative algorithm for now, let's examine it for problems.

If we enter a year less than 1582, we'll give a meaningless result - it would be better to test for that and give an error message. (It's a quite likely user error to enter just the last two digits of the year, for example).

The loop we have has a test for whether it's the last iteration. Instead of doing that, use a loop with one less iteration, and move the last-iteration code after the loop. That looks like this:

    for(int i = 1582; i < year; i++){
        if(isLeapYear(i)){
            days += 366;
        } else {
            days += 365;
        }
    }

    for(int j = 1; j < mounth; j++) {
        if(j == 1 || j == 3 || j == 5 || j == 7 || j == 8 || j == 10 || j == 12){
            days += 31;
        } else if (j == 4 || j == 6 || j == 9 || j == 11){
            days += 30;
        } else if (j == 2){
            if(isLeapYear(year)){
                days += 29;
            } else {
                days += 28;
            }
        }
    }

    days += day;

Doesn't that seem more reasonable?

The big if in the "month" loop might be clearer as a switch - or perhaps a lookup in an array. Be sure to check that 0≤month≤12 first - more error reporting!

---

There's a subtle bug here:

   int a = (days-3)%7;

The first couple of days in 1582 will result in a negative value there. That shows the value of testing "edge cases" - we really should have tried the very first day that must work.

We can ensure a positive value by writing (days+4) % 7 instead. Or ever initialise days with value 4 (or better, with Thursday, corresponding to 1581-12-31) instead of 0, so we don't need this adjustment at the end.

Answer 2

General Observations

The best thing I can say about the code is that at least it doesn't have using namespace std; in the code.

Not sure why one would use std::function for this at all, however, more functions would make this code easier to read, write, debug and maintain.

In C and C++ arrays and enums default to starting at zero (0) rather than 1. This could be used to simplify the code.

There is no error checking in the code and any code that has user input needs error checking. The code doesn't prompt the user to enter a value, nor does it specify the format the user should use.

The scanf() function is a C library function, it would be better to use C++ input such as std::cin. Probably the code should input a string and then convert the string to numbers.

Generally switch/case statements should include a default case for when a proper value is not used.

It isn't clear why the enum Weeknday is created since it isn't actually used.

Magic Numbers

There are a lot of Magic Numbers in the code (1, 3, 5, 7, 12, 30, 31, 28, 29, 365, 366), it might be better to create symbolic constants for them to make the code more readble and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintainence easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Some examples of symbolic constants usage:

constexpr int LEAP_YEAR_COUNT = 366;
constexpr int NORMAL_YEAR_COUNT = 356;
constexpr int LEAP_FEBRUARY_COUNT = 29;
constexpr int FEBRUARY_COUNT = 28;

int main() {

    // Get date
    int day, mounth, year;
    scanf("%d.%d.%d", &day, &mounth, &year);

    // Count days
    int days = 0;
    for (int i = 1582; i <= year; i++) {
        if (i != year) {
            if (isLeapYear(i)) {
                days += LEAP_YEAR_COUNT;
            }
            else {
                days += NORMAL_YEAR_COUNT;
            }
        }
        ...

First Optimization

In some cases you can use conditional assignments rather than if then else

        if (i != year) {
            days += (isLeapYear(i)) ? LEAP_YEAR_COUNT : NORMAL_YEAR_COUNT;
        }

Sometimes this is called a Ternary, it is quite common in C and C++ and in some other programming languages.