# Printing a factorial from an integer

I am very new to functional programming. I wrote this program where the user can enter an integer and it will print the factorial. If the user enters a negative integer or not an integer at all, it'll give the user a second chance.

This doesn't feel very functional to me. How can I make this code more functional (esp. the main function)? Or is this just fine?

module Main where
import Data.Char

factorial :: Integer -> Integer
factorial 0 = 1
factorial n = product [1..n]

isIntegral :: String -> Bool
isIntegral [] = False
isIntegral st = all isDigit st

main :: IO ()
main = do
putStrLn "Enter a non-negative integer:"
numberStr <- getLine
if isIntegral numberStr
then do
let number = read numberStr :: Integer
if number < 0
then do
main
else do
putStrLn $show$ factorial number
else do
main


## 4 Answers

The line factorial 0 = 1 is redundant. The product of the empty list is already 1.

When writing a function that only works with strings (instead of arbitrary lists), I'd prefer to use "" instead of [] to denote the empty string. This way it's more obvious we're dealing with strings.

Since your isIntegral function will return false for negative numbers (because - is not a digit), you should name it something like isNaturalNumber instead.

Instead of applying read to the result of getLine, you can just use readLn. (Edit: since in your latest revision you do error checking on the string before using read, this no longer applies). Likewise you can use print instead of putStrLn and show.

Instead of having multiple $s on the same line, it is usually preferred to combine .s and $s. However using print you only need print $factorial number anyway. There is no point in prefixing a single expression with do, so you should remove the dos from your if and else. Also since your isIntegral/isNaturalNumber function already returns false for negative numbers, there's no need to check whether the result is negative after calling read, so you can get rid of the if-statement and the duplication of calling main recursively from two different places. • I have added error checking (see the updated question) so I guess I can't simply use readLn anymore. Is there a solution for that? Thanks for all the other tips! – rightfold Jan 20 '12 at 21:43 • @WTP: Yes, I noticed that. I adjusted my answer in response to your edit (and yes, with error checking it's no longer possible to use readLn). – sepp2k Jan 20 '12 at 21:50 Reifying a few of sepp2k's suggestions: module Main where import Data.Char factorial :: Integer -> Integer factorial n = product [1..n] isNat :: String -> Bool isNat [] = False isNat st = all isDigit st main :: IO () main = do putStrLn "Enter a non-negative integer:" numberStr <- getLine if isNat numberStr then print . factorial . read$ numberStr
else main


An alternate approach, use Maybes:

module Main where
import Data.Char

factorial :: Integer -> Integer
factorial n = product [1..n]

parseNat :: String -> Maybe Integer
parseNat [] = Nothing
parseNat st | all isDigit st = Just $read st | otherwise = Nothing main :: IO () main = do putStrLn "Enter a non-negative integer:" numberStr <- getLine case parseNat numberStr of Just num -> print$ factorial num
Nothing  -> main


For different control flow, you might like unless and when from Control.Monad.

Also, be aware of hlint, which can sometimes give you tips on being more Haskellish.

First of all, Haskell is pure functional language, so you can not write anything non-functional in it. However, some improvements can be made:

• Instead of making a custom check for isIntegral you can use the reads function, which is does the same thing as read, but instead of crashing returns the empty list on failure.

• Your program consists of two steps: reading a number and printing its factorial. However, the complicated control flow makes it difficult to see. You can make it more explicit.

• It is often beneficial to bring the amount of code involving IO to the minimum because IO is hard to reason about. You can extract the IO-free code into separate functions.

The last two "improvements" increase the code size in this case though, so they are not so useful here. Try writing a more complex program and concerns like this will become relevant.

factorial n = product [1..n]

readNumber :: String -> Maybe Integer
readNumber numberStr = case reads numberStr of
[(number,"")] | number >= 0 -> Just number
_ -> Nothing

getNumber :: IO Integer
getNumber = do
putStrLn "Enter a non-negative integer:"
numberStr <- getLine
case readNumber numberStr of
Nothing -> getNumber
Just number -> return number

main :: IO ()
main = getNumber >>= print . factorial

• I'd use a guard in readNumber: [(number,"")] | number >= 0 -> .... – augustss Jan 30 '12 at 4:20

Consider replacing isIntegral with an inline test:

...
main = do
putStrLn "Enter a non-negative integer:"
numberStr <- getLine
if not (null numberStr) && all isDigit numberStr
...


It's hard to explain, both concisely and correctly, what isIntegral (or as some suggested, isNat or isNaturalNumber) does. Sure, you can say it tests if the string contains a non-negative integer. But does it accept hexadecimal syntax? How does it treat octals?

On the other hand, perhaps the definition of isIntegral can be based on purpose, rather than function:

-- | Test if the string is a valid, non-negative integer that can be parsed
-- with 'read'.
isIntegral :: String -> Bool
isIntegral [] = False
isIntegral st = all isDigit st


By this interpretation, keeping isIntegral separate makes it easier to extend later (e.g. to allow hexadecimal syntax, which read :: String -> Integer accepts). It's a matter of fixing a standalone function, rather than modifying code buried in a procedure with broader scope (namely, main).

However, isIntegral's definition assumes that the caller calls read (or similar) after validating the input with isIntegral. It would be simpler to do the reading and validation in one function. See the readNumber function in Rotsor's answer, which uses reads and a guard to accomplish this.

## Playing with point-free

Let's see if we can make this more concise:

isIntegral :: String -> Bool
isIntegral [] = False
isIntegral st = all isDigit st


Or, put another way:

isIntegral st = not (null st) && all isDigit st


We can express it in point-free style (i.e. eliminate variables using combinators like ., flip, uncurry, etc.):

isIntegral = liftA2 (&&) (not . null) (all isDigit)


Yes, this is longer and more cryptic. However, with a little practice, it's easy to see that it's combining two predicates, not . null and all isDigit, with the && operator. Since && doesn't like functions as arguments, the noisy liftA2 (from Control.Applicative) is applied to && to combine predicates.

Although I would say point-free is overkill here, it's good to practice using it. Here are a couple examples where point-free does make things more concise (in my opinion):

-- Read two lines, and combine their values with (+)
addLines :: IO Integer
addLines = (+) <\$> readLn <*> readLn

-- Call "hPutStrLn msg" on each Handle in the list.
--
-- Unfortunately, hPutStrLn takes the Handle argument first, so we have to
-- flip it to use it like we want.
broadcast :: String -> [Handle] -> IO ()
broadcast = mapM_ . flip hPutStrLn


Learning to write in point-free style can help you write programs faster. Here's a script I wrote recently that takes a list of file names, and prints lines present in every file:

import Data.List (foldl1')
import System.Environment

import qualified Data.Set as S

main :: IO ()
main = getArgs
>>= mapM (fmap (S.fromList . lines) . readFile)
>>= mapM_ putStrLn . S.toList . foldl1' S.intersection