8
\$\begingroup\$

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
\$\endgroup\$

4 Answers 4

7
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ 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! \$\endgroup\$
    – user36
    Jan 20, 2012 at 21:43
  • 1
    \$\begingroup\$ @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). \$\endgroup\$
    – sepp2k
    Jan 20, 2012 at 21:50
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I'd use a guard in readNumber: [(number,"")] | number >= 0 -> .... \$\endgroup\$
    – augustss
    Jan 30, 2012 at 4:20
1
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.