2
\$\begingroup\$

I don't have much experience with Haskell or functional programming, but I wrote a working todo list. My main question is if there's any idiomatic shortcuts I could take and if my code is idiomatic enough, but all constructive criticism is welcome. Tested in GHCi 7.6.3:

import Data.Char
import System.IO

numberList :: [String] -> [String]
numberList = zipWith (\a b -> a ++ ". " ++ b) (map show [1..])

removeElem :: Int -> [a] -> [a]
removeElem i xs = take i xs ++ drop (succ i) xs

printStrs :: [String] -> IO ()
printStrs = mapM_ putStrLn

todoOp :: [String] -> Char -> IO [String]
todoOp xs 'V' = do
  if (not.null) xs then printStrs $ numberList xs
  else putStrLn "No entries"
  return xs
todoOp xs 'A' = do
  putStrLn "What would you like to add?"
  str <- getLine
  return (str:xs)
todoOp xs 'D' = do
  if null xs then do
    putStrLn "No entries"
    return xs
  else do
    printStrs $ numberList xs
    putStr "Which line do you want to remove? "
    i <- readLn :: IO Int
    return (removeElem (pred i) xs)
todoOp xs _ = do
    putStrLn "Operation not supported"
    return xs

mainLoop xs = do
  putStr "Do you wish to [a]dd, [d]elete, [v]iew, or [e]xit? "
  c <- getChar
  putChar '\n'
  if (toUpper c) == 'E' then return ()
  else do
    xs' <- todoOp xs (toUpper c)
    mainLoop xs'

main :: IO ()
main = do
    mainLoop []
\$\endgroup\$
2
\$\begingroup\$

There are some points that can be improved:

  • a type synonym like type TodoList = [String] makes it easier to change the type later
  • the IO and non-IO part in todoOp could be split
  • System.IO is not used (getChar and putChar are in Prelude)
  • mainLoop is missing a type signature
  • removeElem can be written with splitAt
  • show can be applied in zipWith's first argument, no need for an additional map there

If we apply those improvements, we end up with

import Data.Char (toUpper)

type Todo     = String
type TodoList = [Todo]

numberList :: TodoList -> [String]
numberList = zipWith (\a b -> show a ++ ". " ++ b) [1..]

removeElem :: Int -> [a] -> [a]
removeElem i xs = let (as, bs) = splitAt i xs
                  in as ++ drop 1 bs

printStrs :: [String] -> IO ()
printStrs = mapM_ putStrLn

--------------------------------------------
-- new functions to have todoOp just deal --
-- with IO and have testable functions    --

showTodoList :: TodoList -> String
showTodoList [] = "No entries"
showTodoList xs = unlines . numberList

addTodo :: TodoList -> Todo -> TodoList
addTodo = flip (:)

removeTodo :: TodoList -> Int -> TodoList
removeTodo = flip removeElem

--------------------------------------------

-- in case your GHC is too old, import Data.Functor for <$>

todoOp :: TodoList -> Char -> IO TodoList
todoOp xs 'V' = putStrLn (showTodoList xs) >> return xs
todoOp xs 'A' = do
  putStrLn "What would you like to add?"
  addTodo xs <$> getLine
todoOp [] 'D' = putStrLn (showTodoList []) >> return []
todoOp xs 'D' = do
    printStrs $ numberList xs
    putStr "Which line do you want to remove? "
    removeTodo xs <$> readLn
todoOp xs _ = do
    putStrLn "Operation not supported"
    return xs

If we're going for brevitiy, a prompt function might come handy, but that's a matter of preference:

prompt :: String -> IO String
prompt xs = putStrLn xs >> getLine

todoOp :: TodoList -> Char -> IO TodoList
todoOp xs 'V' = printTodoList xs >> return xs
todoOp xs 'A' = addTodo xs <$> prompt "What would you like to add?"
todoOp [] 'D' = putStrLn (showTodoList []) >> return []
todoOp xs 'D' = printTodoList xs >> removeTodo xs <$> prompt "Which line do you want to remove? "
todoOp xs _   = putStrLn "Operation not supported" >> return xs

However, there is still one thing amiss. todoOp has to handle every Char, although that shouldn't be necessary. There are only four actions: add, show, remove and quit. The latest does not actually act on a todo list, so let us focus on the first three. We should model them into a type:

data Action = Add | Delete | View

charToAction :: Char -> Maybe Action
charToAction x = case toUpper x of
                   'A' -> Just Add
                   'V' -> Just View
                   'D' -> Just Delete
                   _   -> Nothing

We have to adjust todoOp of course, but that's left as an exercise. mainLoop will now handle the parsing:

mainLoop :: TodoList -> IO ()
mainLoop xs = do
  putStr "Do you wish to [a]dd, [d]elete, [v]iew, or [e]xit? "
  c <- getChar
  putStrLn ""

  case charToAction c of
    Nothing | toUpper c == 'E' -> return ()
    Nothing   -> putStrLn "Unsupported operation" >> mainLoop xs
    Just act  -> todoOp xs act >>= mainLoop

Alternatively, you can use when from Control.Monad:

mainLoop :: TodoList -> IO ()
mainLoop xs = do
  putStr "Do you wish to [a]dd, [d]elete, [v]iew, or [e]xit? "
  c <- getChar
  putStrLn ""

  when (toUpper c /= 'E') $ case charToAction c of
    Nothing   -> putStrLn "Unsupported operation" >> mainLoop xs
    Just act  -> todoOp xs act >>= mainLoop
\$\endgroup\$
  • \$\begingroup\$ Nice, thanks for bringing me further into the magical world of Haskell! Just curious, would a todoOp with the Action type look the same, but matching on the actions instead of Chars and no edge case? \$\endgroup\$ – robbie Nov 6 '17 at 4:05
  • \$\begingroup\$ @robbie0630 exactly \$\endgroup\$ – Zeta Nov 6 '17 at 7:40

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.