3

I'm just starting out with Haskell and hammered out this simple recursive algo to find the LCM for every number in a list. It works, but it's messy and I was hoping for some peer-review on how to make this more elegant, readable, and Haskell-y.

lcms list 
  | length list > 1 = lcms (lcm (head list) (head (tail list)):(tail (tail list)))
  | otherwise = list

So that takes a list and does LCM of the first two items, then prepends it to list minus those two elements. Basically, the psudocode I'm going for is like this:

lcms [a,b,c] = lcm (a, (lcm (b, c))

Any suggestions, anyone? I'm eager to improve at Haskell and write things that people can actually read. Efficiency tips are welcome too!

Thanks, all!

catleeball
  • 781
  • 7
  • 16

2 Answers2

9

It's a fold:

import Data.List

lcms :: [Int] -> Int
lcms xs = foldl' lcm 1 xs

where lcm compute the lcm of just two numbers:

lcm :: Int -> Int -> Int
ErikR
  • 51,541
  • 9
  • 73
  • 124
  • Oh cool, perfect, thank you! I need to study up on folds and get used to using them more. As a follow-up question: is there a reason to use the lazy foldl from prelude over the more eager foldl' from Data.List? Is either faster? I know the lazy foldl can create stack overflows on large lists, but don't know what advantage it provides or of its laziness is faster for short lists. – catleeball Aug 23 '15 at 03:09
  • 1
    You could also use `foldr`, as in `lcms = foldr lcm 1` – amar47shah Aug 23 '15 at 03:09
  • 2
    @clball `foldl` is almost never what you want. – Daniel Wagner Aug 23 '15 at 03:10
  • 1
    This article has an in-depth treatment of the three, with some rules of thumb at the end. https://wiki.haskell.org/Foldr_Foldl_Foldl%27 – amar47shah Aug 23 '15 at 03:14
  • @Daniel Wagner - what about `foldl'` here? – ErikR Aug 23 '15 at 03:32
  • 2
    @user5402 `foldr` and `foldl'` could both be reasonable choices, depending on the usage. Considering that this particular use is type-restricted to `Int` -- an inherently strict type -- I think `foldl'` is the natural choice. – Daniel Wagner Aug 23 '15 at 06:37
  • folder could be used for lazy naturals. – PyRulez Aug 23 '15 at 19:11
  • 1
    @PyRulez, I eagerly await your performance analysis of your preferred LCM algorithm for lists of lazy naturals. – dfeuer Dec 29 '15 at 23:49
3

You can write it with almost the syntax you suggested, thus:

lcms (a:b:c) = lcms (lcm a b:c)
lcms list = list

I find the second clause a little bit odd, but not terrible: it gracefully handles empty lists, though returning a list when you know you will return at most one item might be seen by some hasochists as being a bit imprecise with your types. You could also consider using Maybe, the canonical 0-or-1 element type:

lcms (a:b:c)  = lcms (lcm a b:c)
lcms [answer] = Just answer
lcms []       = Nothing

Another good choice would be to identify a sane base case. For binary operations, the unit of the operation is usually a good choice, so for lcm, I would choose 1, thus:

lcms (a:b:c)  = lcms (lcm a b:c)
lcms [answer] = answer
lcms []       = 1

Generally, one tries to avoid explicit recursion when possible; the other answer shows how to take that step. In this case, there is an intermediate transformation that makes the base cases slightly more aesthetically pleasing: instead of keeping your accumulator at the head of the list -- which only incidentally works because your accumulator is the same type as the list elements -- one can make the accumulation more explicit or less. Thus, one of these:

lcms (x:xs) = lcm x (lcm xs)
lcms []     = 1

-- OR

lcms = go 1 where
    go acc (x:xs) = go (lcm acc x) xs
    go acc []     = acc

These two implementations correspond to choosing foldr or foldl for eliminating the explicit recursion; foldl' would be similar to the second one, but with an extra seq:

lcms = go 1 where
    go acc (x:xs) = let acc' = lcm acc x in acc' `seq` go acc' xs
    go acc []     = acc
Daniel Wagner
  • 145,880
  • 9
  • 220
  • 380
  • Thanks for a very detailed explanation! Why is explicit recursion generally avoided? Just generally not the best way to go about something? Also it's really cool that you were able to implement a solution that was almost exactly the psudocode. That's pretty rad. – catleeball Aug 23 '15 at 03:37
  • 4
    @clball Generally using functions like `map`, `filter`, and similar (and to a certain extent `foldr`) are preferred over general recursion because there are fewer ways to insert bugs, and because it is easier for readers to understand at a glance. If I see recursion, I'm always looking to see what special cases it has that make it not fit into the standard patterns. – Daniel Wagner Aug 23 '15 at 06:36
  • 1
    @clball Note how this solution avoids `head` and `tail`. These functions are dangerous since they will crash your program if you ever call them on an empty list. You then have a burden to carefully insert guards such as your `length list > 1`. Instead, using pattern matching as above will not crash the program, _unless_ you do not cover all the possible cases. Fortunately, `-Wall` will warn you if a definition is not exhaustive. I find this _very_ helpful. Also, pattern matching can make the code more readable. – chi Aug 23 '15 at 09:06