Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It only takes a minute to sign up.
Sign up to join this community
I wrote a simple function to determine if a year is a leap year or not. It works, but does not feel idiomatic.
isLeapYear :: Int -> Bool
isLeapYear year =
(mod year 400) == 0 || ((mod year 4) == 0 && not ((mod year 100) == 0))
Is there a more "haskelly" way to write this function ?
One thing that always comes to my mind when seeing code like that is one of the first principles in programming that everyone learns.
In your code, the part that stands out is mod year n == 0; let's get away with it first:
isLeapYear :: Int -> Bool
isLeapYear year = divisibleBy 400 || (divisibleBy 4 && not $ divisibleBy 100)
where
divisibleBy x = mod year x
Using such short helpers is very convenient in Haskell. In my opinion it's one of the most important reasons for the code staying readable (contrary to some newcomers' opinions who just pile up code in one giant expression).
Now, what I don't like here is the full name for the year; I know, I know, it's descriptive, but bear with me:
type Year = Int
isLeapYear :: Year -> Bool
isLeapYear y = divisibleBy 400 || (divisibleBy 4 && not (divisibleBy 100))
where
divisibleBy x = mod y x == 0
Now, this is the point where I'd really, really leave it be.
divisibleBy x = mod year x == 0, you mean. Also $ has lower precedence than && so you can't use it there, it'd && divisbleBy 4 and not first, and then apply that to divisibleBy 100. Also I'd reorder this to place the "important" rule at the front: d 4 && not (d 100) || d 400
\$\endgroup\$
Apr 15, 2016 at 12:34
divisibleBy x = mod year x
\$\endgroup\$
Apr 15, 2016 at 16:32
Your code is indeed not idiomatic Haskell. It's not at all clear at first glance how the code works, while something simple is going on.
The correct patten usage:
isLeapYear :: Integer -> Bool
isLeapYear year
| isDivisibleBy 400 = True
| isDivisibleBy 100 = False
| isDivisibleBy 4 = True
| otherwise = False
where isDivisibleBy d = year `mod` d == 0
Notice how it becomes obvious at first glance what's happening in the function.
