1
\$\begingroup\$

I'm working through Yet Another Haskell Tutorial. One of the exercises is to prompt the user to enters numbers, entering a 0 to quit. The program should then both sum and multiply all the given numbers, and then display the factorial of each.

It works, but it's obviously sloppy, with some functions doing more than what is obvious. My main concern is whether it's better to try to format output in the nonpure IO functions, or to do it in the pure functions. Should I separate out calculating the data and formatting the output? Any other tips for how I could tighten this up?

module Main
  where

import System.IO

main = do
  hSetBuffering stdin LineBuffering
  putStrLn "Enter a number, or 0 to quit: "
  numList <- getNumbers
  putStrLn (my_sum numList)
  putStrLn (my_product numList)
  putStrLn (factorial numList)

getNumbers = do
  number <- getLine
  let n = read number :: Int
  if n == 0
    then do 
      return []
    else do
      numList <- getNumbers
      return (n : numList)

my_sum [] = "The sum is 0"
my_sum xs = "The sum is " ++ show (foldr (+) 0 xs)

my_product [] = "The product is 0"
my_product xs = "The product is " ++ show (foldr (*) 1 xs)

factorial (x:xs) = show x ++ " factorial is " ++ show (fac x) ++ "\n"
  ++ factorial xs  
factorial [] = ""

fac 1 = 1
fac x = x * fac (x - 1)
\$\endgroup\$
1
\$\begingroup\$

You should definitely separate the calculation from formatting the output. One of Haskell's key strengths is being able to compose functions. By confounding the definition of your function with the presentation of it's return value, you lose the ability to reuse it elsewhere. For instance, if you wanted to use my_sum to sum all of the numbers you were given, and then find the factorial of the sum, you couldn't with your code as-is without reimplementing both functions! Here's a composable definition of my_sum.

my_sum xs = foldr (+) 0 xs

Now when you want to write out to the command line what operation your program has performed, you can format the output in IO, or call a formatting operation (that's what show is).

main = do
    ...
    putStrLn $ "The sum of the numbers you entered is " ++ show (sum numList)
    ...

Now more general advice, you should include a type signature for all of your top-level definitions. This is important for a few reasons. One, it acts as a check on your understanding of what your functions are doing, the compiler will complain if your signature doesn't match what your function really does. Two, it serves as a surprisingly powerful form of documentation for readers of your code. And three, it can help the compiler infer more of the types you use within functions without needing annotations.

As an example of that last point, look at how you had to annotate the line let n = read number with :: Int. If you had given the compiler enough information to go on elsewhere in your program, it would have been able to deduce that on its own. Here's how.

getNumbers :: IO [Int] -- 1) Type signature for the IO action
getNumbers = do
  number <- getLine
  let n = read number -- 3) The type checker sees that n is a part of the returned list, so it must be an Int
  if n == 0
    then do 
      return []
    else do
      numList <- getNumbers
      return (n : numList) -- 2) Here's the list of Ints we're returning per the type signature

You shouldn't have to set stdin to LineBuffering mode at the start of main. getLine will work correctly by default.

getNumbers isn't properly tail-recursive, the recursive call to getNumbers is followed by that return action. In this example it won't make much of a difference, but in a real program where you can expect a large amount of data this would end up blowing up the stack.

Here's the version I ended up producing taking all of the above into account. Read closely and try to make note of all the differences between both versions!

module Main where

main :: IO ()
main = do
    putStrLn "Enter one number per line, or 0 to stop: "
    numbers <- getNumbers
    putStrLn "The sum of your numbers is " ++ show (my_sum numbers)
    putStrLn "The product of your numbers is " ++ show (my_sum numbers)
    forM_ numbers $ \n -> putStrLn "The factorial of " ++ show n ++ " is " ++ show (fac n)

getNumbers :: IO [Int]
getNumbers = do
    number <- getLine
    let n = read number
    if n == 0
        then return []
        else fmap (n:) getNumbers

my_sum :: [Int] -> Int
my_sum = foldr (+) 0

my_product :: [Int] -> Int
my_product = foldr (*) 1

fac :: Int -> Int
fac 1 = 1
fac n = n * fac (n - 1)
\$\endgroup\$

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.