4
\$\begingroup\$

I implemented the following (what I believe to be) safe Reverse Polish Calculator.

Now I'm using foldM to take advantage of the power of Monads. Previously, I was not.

Can you please critique my code?

import Data.Maybe
import Text.Regex
import Control.Monad
import Network.CGI.Protocol

data Operator = Add | Subtract | Multiply

-- safe Reverse Polish Notation calculator
rpn :: String -> Maybe Double
rpn xs = maybeStringToMaybeDouble result :: Maybe Double
      where result = safeHead $ foldM foldingOp [] $ words xs

--foldM :: Monad m => (a -> b -> m a) -> a -> [b] -> m a

maybeStringToMaybeDouble :: Maybe String -> Maybe Double
maybeStringToMaybeDouble x = do
      y <- x
      maybeRead y 

-- extract maybeHead of Maybe [a]
safeHead :: Maybe [a] -> Maybe a
safeHead m = do
    x <- m -- x has type [a]
    listToMaybe x

foldingOp :: [String] -> String -> Maybe [String]
foldingOp acc s 
   | isNumber s        = Just (s : acc)
   | isPlusRegex s     = performOp Add acc
   | isMinusRegex s    = performOp Subtract acc
   | isMultiplyRegex s = performOp Multiply acc
   | otherwise         = Nothing

performOp :: Operator -> [String] -> Maybe [String]
performOp o (x:y:zs) = Just $ (doOp o x y) : zs
performOp _ _        = Nothing

doOp :: Operator -> String -> String -> String
doOp Add a b       = show $ (read b :: Double) + (read a :: Double)
doOp Subtract a b  = show $ (read b :: Double) - (read a :: Double)
doOp Multiply a b  = show $ (read b :: Double) * (read a :: Double)


isNumber :: String -> Bool
isNumber = matches' mkNumberRegex

mkNumberRegex :: Regex
mkNumberRegex = mkRegex "^[0-9]+[.]?[0-9]*$"

isPlusRegex :: String -> Bool
isPlusRegex = matches' mkPlusRegex

isMinusRegex :: String -> Bool
isMinusRegex = matches' mkMinusRegex

isMultiplyRegex :: String -> Bool
isMultiplyRegex = matches' mkMultiplyRegex

mkPlusRegex :: Regex
mkPlusRegex = mkRegex "^[+]$"

mkMinusRegex :: Regex
mkMinusRegex = mkRegex "^[-]$"

mkMultiplyRegex :: Regex
mkMultiplyRegex = mkRegex "^[*]$"

matches' :: Regex -> String -> Bool
matches' r x 
   | matchRegex r x == Nothing = False
   | otherwise                 = True
\$\endgroup\$
4
\$\begingroup\$

This:

maybeStringToMaybeDouble x = do
      y <- x
      maybeRead y

Could be:

maybeStringToMaybeDouble x = x >>= maybeRead

Similarly,

safeHead m = do
    x <- m
    listToMaybe x

Could be

safeHead m = m >>= listToMaybe

Or we could remove these functions entirely and just write

rpn xs = foldM foldingOp [] (words xs) >>= listToMaybe >>= maybeRead

It seems strange that we're importing Network.CGI.Protocol for maybeRead. I would suggest using readMaybe from Text.Read instead.

There's a chance to use isJust here:

matches' :: Regex -> String -> Bool
matches' r x 
   | matchRegex r x == Nothing = False
   | otherwise                 = True
matches' :: Regex -> String -> Bool
matches' r x = isJust $ matchRegex r x

We can do away with six functions by rewriting foldingOp like this:

foldingOp :: [String] -> String -> Maybe [String]
foldingOp acc s 
   | isNumber s = Just (s : acc)
   | s == "+"   = performOp Add acc
   | s == "-"   = performOp Subtract acc
   | s == "*"   = performOp Multiply acc
   | otherwise  = Nothing

And we can simplify further by removing the Operator type:

foldingOp :: [String] -> String -> Maybe [String]
foldingOp acc s 
   | isNumber s = Just (s : acc)
   | s == "+"   = performOp (+) acc
   | s == "-"   = performOp (-) acc
   | s == "*"   = performOp (*) acc
   | otherwise  = Nothing

performOp :: (Double -> Double -> Double) -> [String] -> Maybe [String]
performOp o (x:y:zs) = Just $ doOp o x y : zs
performOp _ _        = Nothing

doOp :: (Double -> Double -> Double) -> String -> String -> String
doOp op a b = show $ (read b :: Double) `op` (read a :: Double)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.