Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Pretty new to haskell here, I've implemented a very simple in-memory "database" just as an exercise, thought I'd post it here and see if there's anything obvious I should be doing that fits more of the haskell way of doing things.

import qualified Data.Map as M

type History = [String]

data Result = Result (Maybe String) Bool DB

data Command = Command { name  :: String
                       , key   :: Maybe String
                       , value :: Maybe String
                       }

data DB = DB (M.Map String String)

cmdKey c   = let Just s = key   c in s
cmdValue c = let Just s = value c in s

runSetCmd :: Command -> DB -> IO (Maybe String, DB)
runSetCmd c db@(DB map) = do
    let newMap = M.insert (cmdKey c) (cmdValue c) map
    return $ (Nothing, DB newMap)

runGetCmd :: Command -> DB -> IO (Maybe String, DB)
runGetCmd c db@(DB map) = return $ (M.lookup (cmdKey c) map, db)

execCmd :: DB -> Command -> IO Result
execCmd db c@(Command name key value) = do
    (output,newDB) <- case name of
        "set" -> runSetCmd c db
        "get" -> runGetCmd c db
        _     -> return (Nothing, db)
    return $ Result output end newDB

  where
    end = case name of
              "end" -> True
              _     -> False

getCmd = getLine >>= return . parseCmd

parseCmd :: String -> Command
parseCmd s =
    case words s of
        (name:key:value:_) -> Command name (Just key) (Just value)
        (name:key:[])      -> Command name (Just key) Nothing
        (name:[])          -> Command name Nothing Nothing

displayResult :: Result -> IO Result
displayResult r@(Result (Just s) _ _) = putStrLn s >> return r
displayResult r                       = return r

continue :: Result -> IO ()
continue (Result _ end db) = if end then return () else repl db

repl state = getCmd >>= execCmd state >>= displayResult >>= continue

main = repl (DB M.empty)
share|improve this question

3 Answers

up vote 2 down vote accepted

First of all, as Karl said, you should minimize the code in IO monad and make sure most of your code is in pure functions

Second your Command type is not properly designed

data Command = Command { name  :: String
                         , key   :: Maybe String
                         , value :: Maybe String
                       }

Having the name as String is not cool. You could have a type something like

data CommandType = Get | Set

...but still your command type has too many invalid combinations which the type system cannot help you with (for example: name="get", key="xyz", value="asdfasdf"). Your type should be designed such that you avoid these combinations and you make the type system work for you.

Given this I would define the Command type like this:

data Command = Invalid | End | Get String | Set String String

if your type is like this then execCmd will become:

exec (Get key) = ....
exec (Set key value) = ...
exec Exit = ...
exec Invalid = ...

If you only address these two issues you will see a lot of simplification in your code. Other things would be:

Using the State monad will help you with the passing of the current db state between the functions operating on it. If you do this then passing the state will not be your concern and your result could be either a tuple or a record:

data Result = Result { output: String ; end:Bool} 

Then your displayResult will look like this:

displayResult = putStrLn . output
share|improve this answer

runSetCmd, runGetCmd, and execCmd don't need to be in the IO monad.

It feels a bit wrong to have runGetCmd returning a "new DB". Of course, it really returns the same DB, but the caller has no guarantee.

cmdKey and cmdValue are partial functions, which is dangerous. What you could do instead is have a unique data constructor in the Command type for each command, which has exactly the parameters that command needs.

I'd recommend enabling all compiler warnings and making warnings into fatal errors. (That's -Wall -Werror.) It will probably catch some little things in here.

share|improve this answer

You have two important effects going on:

  • IO, to interact with the user

  • State, to store the map

You can combine these using monad transformers, which let you layer State on top of IO by using StateT. When you do, the program becomes much more concise:

import Control.Monad (forever)
import Control.Monad.Trans.Class (lift)
import Control.Monad.Trans.State
import Data.Map as M

loop :: StateT (Map String String) IO r
loop = forever $ do
    l <- lift getLine
    case (words l) of
        ["get", key]      -> do
            mval <- gets (M.lookup key)
            lift $ putStrLn $ case mval of
                Nothing  -> "Invalid key"
                Just val -> val
        ["set", key, val] -> modify (M.insert key val)
        _                 -> lift $ putStrLn "Invalid command"

repl :: IO r
repl = evalStateT loop M.empty

If you are new to monad transformers, I highly recommend you read Monad Transformers - Step by Step.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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