When you have exceptions, who needs return?



  • 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.





    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().



  • @foxyshadis said:



    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:

    [code language="c#"]return (year % 400 == 0) || (year % 4 == 0 && year % 100 != 0);[/code]



  • 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 said:

    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.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.