2
\$\begingroup\$

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;


}
\$\endgroup\$
5
  • \$\begingroup\$ Have you turned this assignment in to the instructor yet? \$\endgroup\$
    – pacmaninbw
    Commented Oct 17, 2022 at 13:04
  • \$\begingroup\$ We dont turn it in. We just do it to train, so I can't compare it with any "valid" code. We have separate assigmenets to train, and separate assigments to do with instructor. \$\endgroup\$ Commented Oct 17, 2022 at 13:08
  • \$\begingroup\$ The code isn't working quite correctly, I entered today's date and got Tuesday instead of Monday. We can't review code that isn't working correctly. Fix the bug first. We only review code that is working as expected, there are other sites that will help you debug your code. Please read Where can I get help? and How do I ask a good question?. \$\endgroup\$
    – pacmaninbw
    Commented Oct 17, 2022 at 13:21
  • \$\begingroup\$ Hey, i just executed the code (build with g++) and it works for me. Here is a screenshot: imgur.com/a/BbqXgvz @pacmaninbw \$\endgroup\$ Commented Oct 17, 2022 at 13:58
  • \$\begingroup\$ It works for me, modulo the bug where Sundays aren't printed, as noted in my answer. \$\endgroup\$ Commented Oct 17, 2022 at 16:34

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you very much for you response! Everything you wrote makes perfect sense. I'm correcting errors right now! Thank you! \$\endgroup\$ Commented Oct 17, 2022 at 16:15
  • \$\begingroup\$ Do consider posting your improved code for further review (as a new question - see I improved my code based on the reviews. What next?). \$\endgroup\$ Commented Oct 17, 2022 at 16:34
  • 1
    \$\begingroup\$ The key point here is that we can do this without any loops at all, but I wanted you to use your own skills to work out how, rather than spoon-feeding an answer. \$\endgroup\$ Commented Oct 17, 2022 at 16:35
  • \$\begingroup\$ @Adrian, I spotted another bug, and updated the answer \$\endgroup\$ Commented Oct 17, 2022 at 16:49
  • \$\begingroup\$ I will upload my code, just need to get back home. As for the loop, I know that I can do it without it, but our instructor for now required us to use loops. \$\endgroup\$ Commented Oct 17, 2022 at 17:02
1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ I don't see any std::function - what prompted that comment? \$\endgroup\$ Commented Oct 18, 2022 at 15:08
  • \$\begingroup\$ @TobySpeight it was in the original question. \$\endgroup\$
    – pacmaninbw
    Commented Oct 18, 2022 at 15:09
  • 1
    \$\begingroup\$ Ah, we parsed that differently. There's a space there ("std:: functions"), that I think was significant: I read it as using no functions from the std namespace, rather than specifically std::function. That makes more sense, where it's the calendrical functions that would spoil the exercise. \$\endgroup\$ Commented Oct 18, 2022 at 15:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.