What is wrong with my function

Question

i have the following function that is supposed to return true if the passed argument is a reasonable date and false otherwise. the problem is that it is returning false even for obviously reasonable dates and i can't figure out what is wrong with it. anyone with sharper eyes please help. here it is:

fun reasonable_date(x: int*int*int) =
    if #1 x > 0 andalso #2 x > 0 andalso #2 x <= 12 andalso #3 x > 0 andalso #3 x <= 31 
    then                                
    if #2 x = 1 mod 2 andalso #2 x < 8 andalso #3 x <= 31 then true
         else if #2 x = 0 mod 2 andalso #2 x >= 8 andalso #3 x <= 31 
         then true
     else if #2 x = 0 mod 2 andalso #2 x < 8
     then
         if #2 x = 2 andalso (#3 x = 28 orelse #3 x = 29) then true
         else if #2 x = 0 mod 2 andalso #3 x <= 30 then true
         else false
         else if #2 x = 1 mod 2 andalso #2 x > 8 andalso #3 x <=30 then true 
     else false
     else false

Do you mean that it always returns false, or that it returns false for some, but not all, reasonable dates? — Michael J. Barber, Jan 23 '13 at 16:12
You might consider replacing some of the `if-then-else` expressions with appropriate `andalso` or `orelse`. It's quite a tangle, at the moment. — Michael J. Barber, Jan 23 '13 at 16:22
it's always returning false. but now it is ok. It was fixed. thanks — guthik, Apr 29 '15 at 09:13

score 2 · Accepted Answer · answered Jan 23 '13 at 18:33

Your current solution is impossible to maintain, and its logic looks like something that has been to hell and back :)

I would recommend that you break it up into smaller logical parts that ensure simple properties. Thus instead of first testing whether the year, month and day is greater or equal to one, you could group all the logic regarding years, months and days for itself

fun daysInMonth n =
    List.nth([31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31], n-1)

fun reasonable_date (y, m, d) =
    (* Check year >= 1 *)
    y >= 1 andalso

    (* Check 1 <= month <= 12 *)
    (m >= 1 andalso m <= 12) andalso

    (* Check 1 <= day <= n, for n being the number of days in specified month *)
    (d >= 1 andalso d <= daysInMonth m)

Obviously this doesn't handle leap years, however that is also quite simple to implement using a helper function, if the month is February. It could be done like this

fun reasonable_date (y, m, d) =
    (* Check year >= 1 *)
    y >= 1 andalso

    (* Check 1 <= month <= 12 *)
    (m >= 1 andalso m <= 12) andalso

    (* Check 1 <= day <= n, for n being the number of days in specified month *)
    (d >= 1 andalso
     (* If February, and leap year *)
     ((m = 2 andalso isLeapYear y andalso d <= 29)
      (* Any other month or non leap year *)
      orelse d <= daysInMonth m))

score 1 · Answer 2 · answered Jan 23 '13 at 16:22

You repeatedly use conditions like if #2 x = 1 mod 2. This is almost certainly does not work as you think it does. Here, mod is an arithmetic operator, meaning the remainder obtained when dividing 1 by 2, and not the mathematical expression saying that #2 x equals 1 modulo 2. Thus, instead of testing whether #2 x is odd, you're testing whether it equals 1. Following through your conditions, you really only allow true when #2 x is 1, so your reasonable dates must all be in January (and there may not even be any, I haven't worked through all conditions).

Thanks, that was useful. :) it is fixed and now am gonna remodel it to handle even leap years :) — guthik, Jan 23 '13 at 17:26

score -1 · Answer 3 · answered Jan 24 '13 at 08:26

i prefer this solution which seems more readable

fun reasonable_date (y, m, d) =
    let val daysInMonth =
           List.nth([31, if isLeapYear y then 29 else 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31], m-1)
    in
        (* Check year >= 1 *)
        y >= 1 andalso

        (* Check 1 <= month <= 12 *)
        (m >= 1 andalso m <= 12) andalso

        (* Check 1 <= day <= n, for n being the number of days in specified month *)
        (d >= 1 andalso d <= daysInMonth)
    end

but may be i missed some tricks (i assume you already wrote an helper function isLeapYear)

What is wrong with my function

3 Answers3