I sent this to Alex a few months ago but he didn't post it, probably because it's sample code or he has too many others, so I thought I'd share it with y'all.

I found a bug-hunting article on InformIT the other day that asks us to find a minor semantic bug in one of the worst-designed functions I've ever seen.

http://www.informit.com/articles/article.asp?p=342034&seqNum=2

1.  public class IsLeapYear {
2.
3.      public static class LeapYearException
4.              extends Exception {}
5.      public static class NotLeapYearException
6.              extends Exception {}
7.
8.      static void checkLeapYear(String year)
9.          throws LeapYearException, NotLeapYearException,
10.                NumberFormatException {
11.
12.         long yearAsLong = Long.parseLong(year);
13.
14.         //
15.         // A leap year is a multiple of 4, unless it is
16.         // a multiple of 100, unless it is a multiple of
17.         // 400.
18.         //
19.         // We calculate the three values, then make a
20.         // 3-bit binary value out of them and look it up
21.         // in results.
22.         //
23.
24.         final boolean results[] =
25.                 { true, false, false, true,
26.                   false, false, false, false };
27.
28.         if (results[
29.             ((((yearAsLong % 4) == 0) ? 1 : 0) << 2) +
30.             ((((yearAsLong % 100) == 0) ? 1 : 0) << 1) +
31.             ((((yearAsLong % 400) == 0) ? 1 : 0) << 0)]) {
32.             throw new LeapYearException();
33.         } else {
34.             throw new NotLeapYearException();
35.         }
36.     }
37.
38.     public static void main(String[] args) {
39.
40.         if (args.length > 0) {
41.
42.             try {
43.                 checkLeapYear(args[0]);
44.             } catch ( NumberFormatException nfe ) {
45.                 System.out.println(
46.                     "Invalid argument: " +
47.                     nfe.getMessage());
48.             } catch ( LeapYearException lye ) {
49.                 System.out.println(
50.                     args[0] + " is a leap year");
51.             } catch ( NotLeapYearException nlye ) {
52.                 System.out.println(
53.                     args[0] + " is not a leap year");
54.             }
55.         }
56.     }
57. }

Whoa, that was awesome, Firefox pasted it in formatted and everything. Cool.

The error is that the table lookup is reversed, and gives the wrong answer every time. The real bug... worst use of exceptions, worst use of bitshifts, and all this complexity for the equivelent of a one-line C macro. Can you say "just because I could"?

return (year % 4 == 0 && year % 100 != 0 && year % 400 == 0); does the job quite nicely in my experience. (You'll want to test that it's a number and not null first.) Or Date.isLeapYear().

oh my... this must have been a genious at work

Exceptions are rather resource-heavy, and should only be used in real error cases, not thrown at random... I'm glad you have a boolean filetype in Java

And I love the way he calculates the offset in the array using bitwise operators... At least he should have used | instead of + :D

Anyway all the WTF's in this code:
- Exceptions instead of booleans
- calculating an offset in a constant array to return a boolean using bitwise operations
- Bitwise OR'ing with + (offcourse in this case it will work)

Anyway - a very good sample on "how not to code"

foxyshadis wrote:

return (year % 4 == 0 && year % 100 != 0 && year % 400 == 0); does the job quite nicely in my experience. (You'll want to test that it's a number and not null first.) Or Date.isLeapYear().

Let year = 4
year % 4 == 0 (true)
year % 100 != 0 (true)
year % 400 == 4 != 0 (false)

Let year = 400
year % 4 == 0 (true)
year % 100 == 0 (false)
year % 400 == 0 (true)

Well, at least it doesn't get any false positives.

This code might work a bit better:

return (year % 400 == 0) || (year % 4 == 0 && year % 100 != 0);
 

Hey that's a pretty good one; not sure how that one didn't get posted. I'm a bit disorganized ...

Oops, I meant an or in there.

I'd hate to be the guy trying to figure out why calling this function always crashes my program with an unhandled exception.

Well, at least it's y2k compliant:

12. long yearAsLong = Long.parseLong(year);

Guaranteed to work to year 2147483648 and beyond...

foxyshadis wrote:

24.         final boolean results[] = 25.                 { true, false, false, true, 26.                   false, false, false, false }; 27. 28.         if (results[ 29.             ((((yearAsLong % 4) == 0) ? 1 : 0) << 2) + 30.             ((((yearAsLong % 100) == 0) ? 1 : 0) << 1) + 31.             ((((yearAsLong % 400) == 0) ? 1 : 0) << 0)]) {

It's quite funny to note that in addition to all the other problems with this code (mentioned by others and the original poster), it's a horribly inefficient use of the array.  All things divisible by 100 are divisible by 4, all by 400 by 100 and 4, etc., so that it's not even possible to look at the indexes (001, 010, 011, 101), so that only four of the 8 entries in the array can ever be indexed...

Really a smart one, this guy.