4
\$\begingroup\$

I'm looking for 3 different answers:

  1. Using Java predefined functions
  2. Converting from string to number
  3. Without the above two (any other types of answers are also welcome)

I am looking for improving my code (as per the above 3 conditions for 3 answers) taken from my repository here.

/*
 * Checks if the year passed is a leap year
 * 
 * @param year; requires the year to have 4 digits
 * @return true if leap, else false
 * 
 */

public static boolean isLeapYear(int year) {
    String testYear = String.valueOf(year);
    if (testYear.charAt(2) == '1' || testYear.charAt(2) == '3' || 
            testYear.charAt(2) == 5 || testYear.charAt(2) == '7' || 
            testYear.charAt(2) == '9') {
        //If the third digit is odd

        if (testYear.charAt(3)=='2'||testYear.charAt(3)=='6'){ 
            return true;    
        }
        else{
            return false;
        }
    }
    else{
        if (testYear.charAt(2) == '0' && testYear.charAt(3) == '0') {
            if(testYear.charAt(0) == '1' || testYear.charAt(0) == '3' || 
                    testYear.charAt(0) == 5 || testYear.charAt(0) == '7' || 
                    testYear.charAt(0) == '9'){
                //If first digit is odd

                if(testYear.charAt(1)=='2'||testYear.charAt(1)=='6'){
                    return true;
                }
                else{
                    return false;
                }
            }

            //If second digit is not odd
            else if(testYear.charAt(1)=='0'||testYear.charAt(1)=='4'||
                    testYear.charAt(1)=='8'){
                return true;
            }
            else
                return false;
        }
        else if (testYear.charAt(3)=='0'||testYear.charAt(3)=='4'||
                testYear.charAt(3)=='8'){
            //If fourth digit is not odd
            return true;
        }
        else
            return false; 
    }
}
\$\endgroup\$
  • 9
    \$\begingroup\$ Do you know the actual math formula for determining a leap year? \$\endgroup\$ – h.j.k. Jul 10 '15 at 10:15
  • 5
    \$\begingroup\$ A minor point: in your GitHub repository, you shouldn't include binaries generated from the source code in the repository. Use a .gitignore file to ignore bin/. \$\endgroup\$ – Olathe Jul 10 '15 at 16:05
25
\$\begingroup\$

Processing the number as a string

Don't. In general, converting a numerical value to a string and using some string processing on what was once a number is a code smell. You of course often need to convert numbers to strings for output and formatting, but mathematical operations should be done mathematically.

If you for instance wanted to check whether the second-last digit is even, you can divide your number by 10 and check the oddity of that result. That's an arithmetic check without involving any string processing.

The correct algorithm

Whenever you approach a problem, you should think about the correct algorithm to solve it, and do some research, if applicable. Leap years for instance can be detected by a much simpler algorithm. Leap years in the Gregorian calendars are years that are divisible by 4 but not 100, unless they are also divisible by 400. So:

If it's not divisible by 4, it's not a leap year.

If it's divisible by 4, it's a leap year if: it's not divisible by 100, or it's divisible by 400.

Now that looks almost like an algorithm in code. Which you can write along the lines of

if (year % 4 == 0) {
    if (year % 100 != 0) {
        return true; //Divisible by 4 and not by 100, leap year
    }
  return (year % 400 == 0); //Divisible by 4 and by 100. Leap year if divisible by 400.
} 
return false; //Not divisible by 4 - not a leap year.

Of course that could also be a one-liner, but this perhaps makes the decision logic a bit more clear.

Java 8

In Java 8, a lot of functionality for handling calendars has been added, such as the Year class. The following would work:

return Year.isLeap(year);

General

It's hard not to notice that your formatting is inconsistent. Sometimes space before opening braces, sometimes not. Sometimes you surround a single statement with braces, sometimes not. Those are largely up to individual preference (although I think space before braces is a must, as is enclosing single statements in braces), but consistency is definitely very important.

Also, as the answer by RubberDuck notes, it's not needed or a good practice to write code of the if (something) return true; else return false type when you can just return the condition directly.

\$\endgroup\$
  • \$\begingroup\$ Nice answer. Consider removing the else-parts of each of your if-conditions. \$\endgroup\$ – rolfl Jul 10 '15 at 11:35
  • 1
    \$\begingroup\$ +1. I looked up the actual Java 8 implementation - Year.isLeap implements it as a one liner: return ((year & 3) == 0) && ((year % 100) != 0 || (year % 400) == 0); \$\endgroup\$ – spike Jul 12 '15 at 2:05
12
\$\begingroup\$

This approach is truly bizarre. A sane test (considering only Gregorian-era rules) would be

public static boolean isLeapYear(int year) {
    return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
}

If you write a simple unit test comparing the two, you'll find that your implementation gives wrong answers for many years that contain the digit 5, because you made a quoting mistake. The mistake appears twice, due to copy-and-pasted code.

This code also suffers from a Y10K problem.

\$\endgroup\$
  • 1
    \$\begingroup\$ Upvoted for succinct implementation. You might tone down the judgmental language, though! \$\endgroup\$ – ᴇʟᴇvᴀтᴇ Jul 10 '15 at 16:30
  • 2
    \$\begingroup\$ +1 for noticing the Y10K. (There's also a pre-1000 problem, but that is out of range for the Gregorian calender anyway) \$\endgroup\$ – Hagen von Eitzen Jul 10 '15 at 17:18
10
\$\begingroup\$

It was hard not to see this at a glance.

   if (testYear.charAt(3)=='2'||testYear.charAt(3)=='6'){ 
       return true;    
   }
   else{
       return false;
   }

Anytime you find yourself saying "if condition return true, else return false" you can directly return the result of the expression.

return (testYear.charAt(3)=='2'||testYear.charAt(3)=='6'); 
\$\endgroup\$
2
\$\begingroup\$
    if (testYear.charAt(2) == '1' || testYear.charAt(2) == '3' || 
        testYear.charAt(2) == 5 || testYear.charAt(2) == '7' || 
        testYear.charAt(2) == '9') {

An easier way to write this would be simply

    if (year % 20 != 0) {

Which would also work for years 10,000 and up.

Note: I'm not arguing for this approach. I'm just pointing out that if you do have to find years in odd decades, there's a simpler, more efficient way to do it.

\$\endgroup\$
-2
\$\begingroup\$

You can also do it using the java.util.calendar library.

A year is a leap year if:

  1. The remainder is 0 when divided by 4
  2. February has 29 days instead of 28 days

We saw ideas checking when the remainder is 0. I will present an idea of when we want to check if February has 29 days. It is done using the Java built-in library java.util.calendar.

Using the Java Calendar utility, we can set our year to need one and month to 1 (because in Java.util.calendar, months go from 0-11), so 1 is February. We can't set it to any other because only in February we can see it is a leap year.

getActualMaximum(Caledanr.DAY_OF_MONTH) is a function that returns an int value that presents the maximum number of days for a date for which we set the month/year value. If the value is 29 we know that it is a leap year, else it is not a leap year.

It is more effective than your solution, but in some cases other solutions in this question can work better.

public static boolean leap_yr(int year){
Calendar calendar = Calendar.getInstance();
calendar.clear();
calendar.set(Calendar.MONTH, 1);
calendar.set(Calendar.YEAR, year);

if(calendar.getActualMaximum(Calendar.DAY_OF_MONTH) == 29){
 return true;
}
else{
 return false;
}

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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