4
\$\begingroup\$

I have a function to check the leap year as follows

bool IsLeapYear(int year)
{
    if (year % 4 == 0)
    {
        if (year % 100 == 0)
        {
            if (year % 400 == 0)
                return true;
            else
                return false;
        }
        else
            return true;
    }
    else
        return false;
}

I was wondering whether it would be correct to change this function by using guard clauses like below

bool IsLeapYear(int year)
{
    if (year % 4 != 0)
    {
        return false;
    }

    if (year % 4 == 0 && year % 100 == 0 && year % 400 != 0)
    {
        return false;
    }

    return true;    
}

Is it going to introduce any side effects by using guard clause like this or is there a better way to write this function?

\$\endgroup\$

5 Answers 5

6
\$\begingroup\$

Compilers

Welcome to Code Review. First of all, "better" is a very personal opinion term, unless it has been defined. From a compiler's point of view, both functions do the same. As compilers are data flow analysis beasts, a decent one will simplify the function's flow to the following variant (see https://godbolt.org/z/yZCPOv):

ret = 0
if year & 3 /= 0 
   goto end
ret = 1
if year % 100 /= 0
   goto end
ret = (year % 400 == 0)

end: 
   return ret
return 0

As you can see, with -Os (or any other optimization level), your functions will result in the same assembler. This already answers your first (implicit) question:

I was wondering whether it would be correct to change this function by using guard clauses like below

"Better"

Since both generate the same assembler, both are the same. We can of course check that by hand, as there are only four equivalence classes we have to check, but that's left as an exercise.

Regardless, the better way is the one that's easier to understand and follows fits into the rest of your code. If all your code is written in the first style, use that one. If you usually bail out early, use the latter. Neither have advantages over the other as soon as we enable optimizations. Without optimizations, your latter variant uses less jumps (but only a single one), and that's still compiler dependent.

In the end, use the variant you like most, but make sure you can still understand and explain it to yourself or someone else at a later time.

Just for completion, I personally prefer variants that have less jumps but are still readable, so I'd probably write something along

//! @returns whether @c year is a leap year
bool IsLeapYear(int year)
{
    //! A year is a leap year if its divisible by 4, but not by 100,
    //! @b unless it's also divisible by 400.
    return  (year %   4 == 0)
        && ((year % 100 != 0) || (year % 400 == 0));
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ There is also C++20 standard library solution coming. \$\endgroup\$ Oct 27, 2018 at 11:57
1
\$\begingroup\$

Using guard clauses often makes the code more readable and code is read more often than it is written. Some coding standards are not so happy with multiple return statements, but I would definitely go for readability here. If going for guard clauses fully the code would read

bool IsLeapYear(int year)
{
    if (year % 400 == 0)
        return true;

    if (year % 100 == 0)
        return false;

    if (year % 4 == 0)
        return true;

    return false;
}

I think that is easy to read and understand.

\$\endgroup\$
1
  • \$\begingroup\$ I would combine the last three lines into one return return year % 4 == 0; \$\endgroup\$ Oct 31, 2018 at 19:36
1
\$\begingroup\$

I would modify your second version as follows:

#include <stdexcept>
bool IsLeapYear(int year)
{
    if ( year < 1583 ) {
    throw std::invalid_argument( "Year predates inception of Gregorian calendar" );
}
    if (year % 4 != 0)
    {
        return false;
    }

    if (year % 4 == 0 && year % 100 == 0 && year % 400 != 0)
    {
        return false;
    }

    return true;    
}

The modification throws an invalid_argument exception if the year is < 1583. The algorithm in the original post only works for the Gregorian calendar, and that calendar was first used in 1582. One should be careful, however, since some countries switched from Julian to Gregorian calendars much later (Greece didn't switch until 1923).

Although the rules for the Gregorian calendars could be projected to years before the Gregorian calendar was adopted, historians usually don't; they usually use the calendar that was in force at the time and place they are writing about, if the calendar was Roman, Julian, or Gregorian.

\$\endgroup\$
3
  • \$\begingroup\$ I would remove the if ( year < 1583 ). If you do want it then add if (isGregorianCalendars(year, location)). Also note the Julian Calendar (just before the gregorian) also had leap years (just not the one every 400 years) and the revised Julian Calendar had leap years the same as the Gregorian (with the first split happening in 2800). \$\endgroup\$ Oct 31, 2018 at 19:40
  • \$\begingroup\$ I am unable to find any information for isGregorianCalendars for c++ \$\endgroup\$ Nov 1, 2018 at 0:52
  • \$\begingroup\$ There is none. I was saying that if ( year < 1583 ) is 1) not accurate 2) not flexible. Therefore you should encapsulate the concept in its own named function so documenting your entente and providing a way for a maintainer to update and fix. \$\endgroup\$ Nov 1, 2018 at 3:46
0
\$\begingroup\$

IMHO, i find this more readable:

bool is_leap(unsigned int year) {
  return !(year % 4) && ((year % 100) || !(year % 400));
}

Or, in reverse order:

bool is_leap(unsigned int year) {  
  return !(year % 400) || ((year % 100) && !(year % 4));
}
\$\endgroup\$
2
  • \$\begingroup\$ That's definitely not more readable. Its more compact. I have to get out a piece of paper and walk through that to validate it is correct. Also am a little curious if its more expansive. \$\endgroup\$ Oct 31, 2018 at 19:38
  • \$\begingroup\$ It was just MY opinion. When I see !(year % 4) && ((year % 100) || !(year % 400)) I read "divisible by 4 and not divisible by 100 unless also divisible by 400". For me it's very fluent, but I'm probably weird. Another option would be to write an inline function bool divisible(int, int) that do the same job. \$\endgroup\$
    – Calak
    Oct 31, 2018 at 19:52
0
\$\begingroup\$

I like the simpler:

bool leapYear(int year)
{
     if (!isGregorianCalendars()) {
         throw std::runtime_error("Can't tell you. Calendars are hard");
     }

     if (year % 400 == 0) {
         return true;
     }
     if (year % 100 == 0) {
         return false;
     }
     return year % 4 == 0;
}
\$\endgroup\$

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.