Function for checking leap years

Question

As part of one of my AP Computer Science labs, I was required to write a function to determine whether a year was a leap year or not. Included as comments are a few reasons why I did things, as well as a more detailed version of the rules for leap years. Is there anything I could do better or have forgotten?

/** 
 * Returns if the current year is a leap year
 */
private boolean isLeapYear() 
{
    // No leap years occured before 1582 because that is when they were added to the calendar
    if(year > 1582)
    {
        // All code past here only executed when year mod 4 is true
        if(year % 4 != 0) 
        {
            return false;
        }

        // Makes sure that it does not register the beginning of centuries as leap years
        // unless they actually are.
        else if(year % 100 == 0 && year % 400 != 0) 
        {
            return false;
        }
        // Also is the same as checking for mod 4 and mod 100, since they are factors of 400
        else if(year % 400 == 0)
        {
            return true;
        }
        else 
        {
            return true;
        }

        /*
        • A year that is not divisible by 4. - NOT LEAP YEAR

        • A year that is divisible by 4 but not divisible by 100 or 400. - LEAP YEAR

        • A year that is divisible by 4 and 100 but not 400. - NOT LEAP YEAR

        • A year that is divisible by 4 and 100 and 400. - LEAP YEAR

        • A year prior to 1582 that is a leap year. - NOT A LEAP YEAR

        • A year prior to 1582 that is not a leap year. - NOT A LEAP YEAR */
    }
    return false;
}

Answer 1

This question of leap years is an interesting one... and seems to be a rite-of-passage (when you do Zeller's Congruence, you should get that reviewed too... ;-)

In Java, (and other languages), it is common to return-early. You can use this to your advantage to reduce code-nesting, by choosing whether to do a negative, or positive test in if conditionals. You have done this a couple of times in your code, but not always. For example, your code contains:

// No leap years occured before 1582 because that is when they were added to the calendar
if(year > 1582)
{
    // All code past here only executed when year mod 4 is true
    if(year % 4 != 0) 
    {
        return false;
    }

You have a lot of code inside that "if" condition block, but, if you reverse the logic:

// No leap years occured before 1582 because that is when they were added to the calendar
if(year <= 1582) {
    return false;
}

// All code past here only executed when year mod 4 is true
if(year % 4 != 0) 
{
    return false;
}

You have immediately 'un-nested' a large portion of your code, which makes readability better.

When you write code to match a specification, like you have here, it is valuable to include the specification you used inside your code, which, you have also done... but, do that before the code, not after it.

If you have an if-else block, and the if-side (the true side) always does a return, then there is no need for the else. This can simplify the code further....

Performing these changes, we get to:

private boolean isOPLeapYear() {
    /*
     • A year that is not divisible by 4. - NOT LEAP YEAR
     • A year that is divisible by 4 but not divisible by 100 or 400. - LEAP YEAR
     • A year that is divisible by 4 and 100 but not 400. - NOT LEAP YEAR
     • A year that is divisible by 4 and 100 and 400. - LEAP YEAR 
     • A year prior to 1582 that is a leap year. - NOT A LEAP YEAR 
     • A year prior to 1582 that is not a leap year. - NOT A LEAP YEAR
     */

    // No leap years occurred before 1582 because that is when they were
    // added to the calendar
    if (year <= 1582) {
        return false;
    }
    // All code past here only executed when year mod 4 is true
    if (year % 4 != 0) {
        return false;
    }

    // Makes sure that it does not register the beginning of centuries as
    // leap years
    // unless they actually are.
    if (year % 100 == 0 && year % 400 != 0) {
        return false;
    }
    // Also is the same as checking for mod 4 and mod 100, since they are
    // factors of 400
    if (year % 400 == 0) {
        return true;
    }
    return true;

}

Once we have the code like that, it is apparent that the last % 400 check is unnecessary, and since the comments are repetitive (the // comments duplicate much of what the spec and code say), they can be mostly removed, reducing the code to:

private boolean isOPLeapYear() {
    /*
     • A year that is not divisible by 4. - NOT LEAP YEAR
     • A year that is divisible by 4 but not divisible by 100 or 400. - LEAP YEAR
     • A year that is divisible by 4 and 100 but not 400. - NOT LEAP YEAR
     • A year that is divisible by 4 and 100 and 400. - LEAP YEAR 
     • A year prior to 1582 that is a leap year. - NOT A LEAP YEAR 
     • A year prior to 1582 that is not a leap year. - NOT A LEAP YEAR
     */

    if (year <= 1582) {
        return false;
    }

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

    if (year % 400 == 0) {
        return true;
    }

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

    return true;

}

The above code is the same logic as yours, but.... more readable, that's all.

Answer 2

You’re checking year % 400 twice here, and returning true in both of the last two cases:

else if(year % 100 == 0 && year % 400 != 0) 
{
    return false;
}
// Also is the same as checking for mod 4 and mod 100, since they are factors of 400
else if(year % 400 == 0)
{
    return true;
}
else 
{
    return true;
}

That can be simplified to

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

and since return exits from a function, you don’t need elses at all:

if (year > 1582)
{
    if (year % 4 != 0)
        return false;

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

    return true;
}

return false;

You can invert that 1582 check for consistency and less nesting:

if (year <= 1582)
    return false;

if (year % 4 != 0)
    return false;

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

return true;

May as well invert and combine them at this point:

private boolean isLeapYear() 
{
    return year > 1582 && year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
}

One final consideration is whether a function called isLeapYear() should really be a method of anything (other than a class representing nothing more than a year). Consider making it static and having it accept year as an argument.

Answer 3

Return Early

If you return early, you can avoid nested if statements. It even fits in very well with the comment you already have:

// No leap years occured before 1582 because that is when they were added to the calendar
if(year <= 1582) {
    return false;
}

if(year % 4 != 0) {
[...]

Further Simplify if statements

You could also save some comments and an if-clause if you save the conditions in variables:

boolean isBeginningOfCentury = year % 100 == 0;
boolean isLeapYearCentury = year % 400 == 0;

With this, your code could look like this:

private boolean isLeapYear() {
    // No leap years occured before 1582 because that is when they were added to the calendar
    if(year <= 1582) {
        return false;
    }

    if (year % 4 != 0) {
        return false; // only multiple of 4 can possibly be leap-years
    }

    boolean isBeginningOfCentury = year % 100 == 0;
    boolean isLeapYearCentury = year % 400 == 0;

    if (isBeginningOfCentury && !isLeapYearCentury) {
        return false;
    }

    if (isLeapYearCentury) {
        return true;
    }

    return true;
}

The isLeapYearCentury is not needed, I just left it in for clarity.

You could also simplify it even further to:

private boolean isLeapYear() {
    boolean isBefore1582            = year <= 1582;
    boolean isMultipleOfFour        = year % 4 == 0;
    boolean isBeginningOfCentury    = year % 100 == 0;
    boolean isLeapYearCentury       = year % 400 == 0;

    return isMultipleOfFour && !isBefore1582 && (!isBeginningOfCentury || isLeapYearCentury);
}

The variables are of course not necessary, but I think the return reads a lot nicer with them.

Misc

• your long comment at the end should probably be moved to the top.
• the general Java style is: opening curly brackets go on the same line, if is followed by a whitespace, and else goes on the same line as the closing curly bracket (you don't have to follow conventions, but I would recommend doing so).

Answer 4

A function to check if a year is a leap year or not should be a method and take the year as a parameter, for example as part of a DateUtils class:

public static boolean isLeapYear(int year) {
    // ...
}

If you want to be sure it's really working, I recommend to add some unit tests, for example:

@Test
public void test_NotLeapYear_IfNotDivisibleBy4() {
    assertFalse(isLeapYear(2003));
}

@Test
public void test_LeapYear_IfDivisibleBy_4_Not_100_400() {
    assertTrue(isLeapYear(2004));
    assertFalse(isLeapYear(2100));
}

@Test
public void test_NotLeapYear_IfDivisibleBy_4_100_Not_400() {
    assertTrue(isLeapYear(2000));
    assertFalse(isLeapYear(2100));
    assertFalse(isLeapYear(2200));
    assertFalse(isLeapYear(2300));
    assertTrue(isLeapYear(2400));
}

@Test
public void test_LeapYear_IfDivisibleBy_4_100_400() {
    assertTrue(isLeapYear(2000));
    assertTrue(isLeapYear(2400));
    assertTrue(isLeapYear(2800));
}

@Test
public void test_NotLeapYear_Before1582_EvenIfDivisibleBy_4_100_400() {
    assertFalse(isLeapYear(1000));
    assertFalse(isLeapYear(1400));
}

@Test
public void test_NotLeapYear_Before1582() {
    assertFalse(isLeapYear(1004));
    assertFalse(isLeapYear(1404));
    assertFalse(isLeapYear(1405));
}

@Test
public void test_LeapYear_Loop() {
    for (int year = 2000; year < 3000; ++year) {
        boolean result = isLeapYear(year);
        if (year % 400 == 0) {
            assertTrue(result);
        } else if (year % 100 == 0) {
            assertFalse(result);
        } else if (year % 4 == 0) {
            assertTrue(result);
        } else {
            assertFalse(result);
        }
    }
}

Answer 5

As pointed out by @Mark, you have a misconception of the Gregorian calendar reform. The problem with the previous calendar was not that there were no leap years. Rather, there were too many leap years, such that the seasons were slipping against the calendar by ¾ day per century. Pope Gregory's solution was to cancel the leap days that would otherwise have occurred in 1700, 1800, 1900, 2100, etc.

I suggest writing the code this way:

private boolean isLeapYear() {
    if (year % 100 == 0            // For one leap year per century,
          && year % 400 != 0       // ... except every 400 years,
          && year > 1582) {        // ... since the 1582 decree,
        return false;              // apply Gregorian correction.
    }
    return year % 4 == 0;
}

You could also write the whole function as one big boolean expression, but I don't think that that would enhance readability.