Skip to content

HW 2 init #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: amos
Choose a base branch
from
Open

HW 2 init #5

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
60d0e92
Update Readme to show how to run a single test
adkron Nov 19, 2018
36d6fab
Merge pull request #3 from kcelixir/adkron-patch-1
adkron Nov 20, 2018
a93524d
HW 2 init
adkron Nov 20, 2018
a04e954
Merge remote-tracking branch 'origin/master' into amos2
adkron Nov 21, 2018
d1fce6f
Updates with some review items from @glguy
adkron Nov 21, 2018
5a7db0a
Removes reverse by using foldl instead of foldr
adkron Nov 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 27 additions & 5 deletions src/Cis194/Hw/LogAnalysis.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,44 @@ module Cis194.Hw.LogAnalysis where
import Cis194.Hw.Log
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note about a flaw in this exercise: LogMessage shouldn't have an Unknown constructor. Error handling like this should be done in parallel to the type for valid log messages. At a minimum you might have: Either String LogMessage or something. This causes problems later in the exercises when you have to ignore Unknown.


parseMessage :: String -> LogMessage
parseMessage s = Unknown s
parseMessage s =
case words s of
"E":i:ws -> buildMessage (Error (read i)) ws
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read can be OK when you're sure that the input string will parse, or when you're doing some temporary debugging code, but if you want to detect parse failures check out Text.Read.readMaybe :: Read a => String -> Maybe a

"I":ws -> buildMessage Info ws
"W":ws -> buildMessage Warning ws
_ -> Unknown s

buildMessage :: MessageType -> [String] -> LogMessage
buildMessage m (t:ws) = LogMessage m (read t) (unwords ws)
Copy link

@glguy glguy Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your function only makes sense for non-empty lists, it can be better to instead take the first element as a distinct argument:

buildMessage :: MessageType -> String -> [String] -> LogMessage

and then put the logic for what to do when you don't have one in the caller

"W":w:ws -> buildMessage Warning w ws


parse :: String -> [LogMessage]
parse _ = []
parse = map (parseMessage) . lines
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the parentheses


insert :: LogMessage -> MessageTree -> MessageTree
insert (Unknown _) t = t
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written this pattern is just fine, but a neat thing you can use is record constructor syntax to match a constructor where you don't care about any of its fields. This pays off more for constructors with more fields:

insert Unknown{} t = t

insert l Leaf = Node Leaf l Leaf
insert n@(LogMessage _ nt _) (Node l m@(LogMessage _ mt _) r)
| nt < mt = Node (insert n l) m r
| nt > mt = Node l m (insert n r)
insert _ t = t

build :: [LogMessage] -> MessageTree
build _ = Leaf
build = foldr insert Leaf . reverse
Copy link

@glguy glguy Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foldr combined with reverse behaves like a foldl with an extra pass.


inOrder :: MessageTree -> [LogMessage]
inOrder _ = []
inOrder (Node l m r) = inOrder l ++ [m] ++ inOrder r
inOrder Leaf = []
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implements the algorithm as specified, but it is somewhat inefficient to left-nest ++ as each nested use must be deconstructed by the next ++. A common pattern for this is:

inOrder :: MessageTree -> [LogMessage]
inOrder t = go t []
  where
    go (Node l m r) = inOrder l . (m:) . inOrder r
    go Leaf = id

This pattern is captured by the dlist package as well as the ShowS type in base.


-- whatWentWrong takes an unsorted list of LogMessages, and returns a list of the
-- messages corresponding to any errors with a severity of 50 or greater,
-- sorted by timestamp.
whatWentWrong :: [LogMessage] -> [String]
whatWentWrong _ = []
whatWentWrong = map getString . filter isSever . inOrder . build

isSever :: LogMessage -> Bool
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

severe :-)

isSever (LogMessage (Error s) _ _) = s >= 50
isSever _ = False

getString :: LogMessage -> String
getString (LogMessage _ _ s) = s
getString (Unknown s) = s