Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Redundant imports

###Redundant imports TheThe 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

###Avoid Maybe AllAll 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:

###Patterns are power

Patterns are power

###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:

###Patterns are power

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:

Patterns are power

Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141

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

lang-hs

AltStyle によって変換されたページ (->オリジナル) /