Safe Reverse Polish Notation

Question

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

Answer 1

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)