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)


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
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)