Function for checking leap years

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

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.

StackExchange Code Review Q#65221, answer score: 13