patternMinor
Printing a factorial from an integer
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?
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
mainSolution
The line
When writing a function that only works with strings (instead of arbitrary lists), I'd prefer to use
Since your
Instead of applying
Instead of having multiple
There is no point in prefixing a single expression with
Also since your
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.