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 ?

  • 1
    \$\begingroup\$ Probably at least a dozen or so, but you have to ask yourself whether this really matters :) \$\endgroup\$ – Bartek Banachewicz Apr 15 '16 at 10:57
  • 1
    \$\begingroup\$ I really doesn't, but since I am beginning, I might as well do it the right way :) \$\endgroup\$ – Thomas Francois Apr 15 '16 at 12:35

One thing that always comes to my mind when seeing code like that is one of the first principles in programming that everyone learns.

Don't repeat yourself.

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)
        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))
        divisibleBy x = mod y x == 0

Now, this is the point where I'd really, really leave it be.

| improve this answer | |
  • 1
    \$\begingroup\$ 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\$ – Gurkenglas Apr 15 '16 at 12:34
  • \$\begingroup\$ There's another divisibleBy x = mod year x \$\endgroup\$ – Gurkenglas Apr 15 '16 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.

| improve this answer | |
  • 2
    \$\begingroup\$ I have a problem with this answer. The code is an almost carbon-copy of an answer already provided. Why did you feel it was necessary to post this anyway? \$\endgroup\$ – Mast Mar 11 '19 at 12:13
  • 1
    \$\begingroup\$ @Mast, this answer codereview.stackexchange.com/a/125768/194860 is wrong, it fails for year 2100 for example (year divisible by 100, not divisible by 400). Because order of patterns is wrong. Not to mention that it has syntax error, which makes it not compile. \$\endgroup\$ – zarcode Mar 11 '19 at 19:16
  • 1
    \$\begingroup\$ I'd add more detail why the variant isn't idiomatic Haskell code. \$\endgroup\$ – Zeta Mar 11 '19 at 23:07

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.