Leap year determination using guard clause

Question

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?

Answer 1

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));
}

Answer 2

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.

Answer 3

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.

Answer 4

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));
}

Answer 5

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;
}