Log messages parser

Question

I've been studying Haskell for some time, but I have never used it for real. So, I started following the CIS 194 Course from UPenn. I resolved the second homework but I'm not fully satisfied with the parser I came up with. I think it's way too verbose and I don't like the way the message string is passed around. I am also not sure if the code is being idiomatic.

Is it following Haskell standards and being idiomatic?

{-# OPTIONS_GHC -Wall #-}
module LogAnalysis where

import Log
import Text.Read

getErrorSeverity :: Maybe Int -> Maybe MessageType
getErrorSeverity (Just c) = Just (Error c)
getErrorSeverity _        = Nothing

parseMessageType :: String -> Maybe MessageType
parseMessageType "" = Nothing
parseMessageType m = case words m of
    "I":_   -> Just Info
    "W":_   -> Just Warning
    "E":l:_ -> getErrorSeverity (readMaybe l)
    _       -> Nothing

parseTimeStamp :: String -> Maybe TimeStamp
parseTimeStamp m = case words m of
    "E":_:s:_ -> readMaybe s
    "I":s:_   -> readMaybe s
    "W":s:_   -> readMaybe s
    _         -> Nothing

parseLogContent :: String -> Maybe String
parseLogContent m = case words m of
    "E":_:_:r -> Just (unwords r)
    _:_:r     -> Just (unwords r)
    _         -> Nothing

parseMessage :: String -> LogMessage
parseMessage "" = Unknown ""
parseMessage m = case (parseMessageType m, parseTimeStamp m, parseLogContent m) of
    (Just t, Just ts, Just c) -> LogMessage t ts c
    _      -> Unknown m

Answer 1

Disclaimer: I assume you want to write confident idiomatic Haskell regardless if an assignment guarantees a perfect log file format. Because, in the real world of software development there are no such guarantees. Better to build the habit now while you're learning rather than later when you're pressed to meet a deadline.

In light of my disclaimer, I must disagree with the answer you accepted. It is bad advice to say that one should avoid Maybe. In fact, I would argue that the code you presented is better than the code you accepted as the answer.

Why?

The parseMessage you wrote is a total function as opposed to a partial function. Let's see what others have to say about this:

• Partial functions
• Avoiding partial functions
• Are your functions total?

Now that we agree on the use of total functions let's try to refactor the code while keeping parseMessage a total function.

There are two main ways parseMessage can fail to parse a message:

1. The log line isn't in the info, warning or error format.
2. The timestamp or severity isn't given as an integer.

The 1st problem we can handle via pattern matching:

parseMessage :: String -> LogMessage
parseMessage m =
  case (words m) of
    "I":t:ws   -> undefined
    "W":t:ws   -> undefined
    "E":s:t:ws -> undefined
    _          -> Unknown m

The 2nd problem we can isolate as follows:

parseInt :: String -> Maybe Int
parseInt s =
  case (reads s :: [(Int, String)]) of
    [(n, "")] -> Just n
    _         -> Nothing

Note the use of Maybe to explicitly declare that this function may fail to parse an Int. It is a total function.

To learn more about reads check out this wonderful article.

Let's now make a bold assumption so that we don't have to deal with Maybe explicitly in our code.

Let's assume we always have an Int when it is needed. For e.g. when we need the timestamp as an Int let's assume we have it.

Doing so allows us to write the following functions:

info c t    = LogMessage Info t c
warning c t = LogMessage Warning t c
err c s t   = LogMessage (Error s) t c

Now, this doesn't quite work:

info (unwords ws) (parseInt t)

Because parseInt returns Maybe Int and info only accepts an Int as its 2nd argument. What we really need is:

asInfo :: String -> Maybe Int -> Maybe LogMessage
asInfo _ Nothing -> Nothing
asInfo c (Just t) -> Just (info c t)

Well, that is what liftA and liftA2 can do for us:

asInfo c    = liftA $ info c
asWarning c = liftA $ warning c
asErr c     = liftA2 $ err c

Finally, when we use asInfo (unwords ws) (parseInt t) we don't want to get back Nothing in the case of a failure. We really want to return Unknown m. We can use maybe for that:

maybe (Unknown m) id (asInfo (unwords ws) (parseInt t))

Putting everything together we get:

import Control.Applicative (liftA, liftA2)

parseMessage :: String -> LogMessage
parseMessage m =
  case (words m) of
    "I":t:ws   -> try $ asI (unwords ws) $ parseInt t
    "W":t:ws   -> try $ asW (unwords ws) $ parseInt t
    "E":s:t:ws -> try $ asE (unwords ws) (parseInt s) $ parseInt t
    _          -> u
  where
    try = maybe u id

    asI c = liftA $ i c
    asW c = liftA $ w c
    asE c = liftA2 $ e c

    i c t   = LogMessage Info t c
    w c t   = LogMessage Warning t c
    e c s t = LogMessage (Error s) t c
    u       = Unknown m

parseInt :: String -> Maybe Int
parseInt s =
  case (reads s :: [(Int, String)]) of
    [(n, "")] -> Just n
    _         -> Nothing

Note how the main part of the code captures the essence of the problem we're trying to solve while we still get to explicitly handle any errors that may occur and remain confident about our code.

case (words m) of
    "I":t:ws   -> try $ asI (unwords ws) $ parseInt t
    "W":t:ws   -> try $ asW (unwords ws) $ parseInt t
    "E":s:t:ws -> try $ asE (unwords ws) (parseInt s) $ parseInt t
    _          -> u

That I believe is the beauty of Haskell.

Answer 2

First of all let me stress that I am only doing basic stuff in haskell right now. This means you should take my advice with more than just a grain of salt.
</disclaimer>

Redundant imports

The first thing that ghci warned me about was: "The import Text.Read is redundant, [..]". Let's get rid of that, we won't need it

Avoid Maybe

All the functions you declared here (or almost all that is) use one or multiple Maybes. Maybe makes you have to jump through hoops to get to your data when it's not necessary, especially since the assignment guarantees a specific log format, that doesn't have room for any Maybe. After removing all the Maybes I got following code:

{-# OPTIONS_GHC -Wall #-}
module LogAnalysis where

import Log

getErrorSeverity :: Int -> MessageType
getErrorSeverity c = Error c

parseMessageType :: String -> MessageType
parseMessageType m = case words m of
    "I":_   -> Info
    "W":_   -> Warning
    "E":l:_ -> getErrorSeverity (read l)

parseTimeStamp :: String -> TimeStamp
parseTimeStamp m = case words m of
    "E":_:s:_ -> read s
    "I":s:_   -> read s
    "W":s:_   -> read s

parseLogContent :: String -> String
parseLogContent m = case words m of
    "E":_:_:r -> unwords r
    _:_:r     -> unwords r

parseMessage :: String -> LogMessage
parseMessage "" = Unknown ""
parseMessage m = case (parseMessageType m, parseTimeStamp m, parseLogContent m) of
    (t, ts, c) -> LogMessage t ts c
    _      -> Unknown m

Loading this into ghci gets me a few warnings about non-exhaustive patterns, but that shouldn't be a problem as long as the logfiles adhere to the given spec :)

Patterns are power

What you're doing here is pattern matching over and over and over. You throw away most of the patterns you match. Repeat after me: Patterns are power

The following 6 lines do the whole work of all the other functions you have there:

parseMessage :: String -> LogMessage
parseMessage "" = Unknown ""
parseMessage m = case words m of
    "E":s:t:r -> LogMessage (Error $ read s) (read t) (unwords r)
    "W":t:r -> LogMessage Warning (read t) (unwords r)
    "I":t:r -> LogMessage Info (read t) (unwords r)
    _ -> Unknown m

Last but not least you could try to extract the duplication around (read t) (unwords r), but I'd say that's not necessary