HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Printing a factorial from an integer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
factorialfromprintinginteger

Problem

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

Solution

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.

Context

StackExchange Code Review Q#8018, answer score: 7

Revisions (0)

No revisions yet.