[Haskell-cafe] code review?

Evan Laforge qdunkan at gmail.com
Mon May 23 00:32:55 CEST 2011


I'd keep the line length down to 80 chars, and not use ';'s.

All of that fiddling with buffering, printing, reading results could
be more clearly put into a couple of functions.

'if all == False then return False else return True' is a pretty
confusing way to say 'return all'.  In fact, any time you see 'x ==
True' you can just remove the '== True'.  The whole postAll thing
would be clearer as

postAll <- if all && not first then return all else ask "Post to all?"
post <- if postAll then return True else ask "Post to..?"

Anywhere you have 'head x' or 'x!!0' or a 'case length xs of' that's a
chance to crash.  Don't do that.  You can get rid of the heads by
writing (config:configs) and [] cases for postStatus.  Get rid of the
!!0 by making config into a data type, it looks like 'data Config =
Config { configPostTo :: URL?, configUser :: Maybe String, configPass
:: Maybe String }'.  Then 'pass <- maybe (ask "pass?") return
(configPass config)'.  Of course, why make these things optional at
all?

Looks like the postStatus return value is not used.  It would simplify
it to not return those codes.

I don't know anything about 'postPlurk' but it looks like it could
return a real data type as well.

All this nested if else stuff makes it hard to read, but I think you
can replace the explicit recursion in postStatus with 'mapM_
(postStatus update) configs'.  It looks like that mysterious (all,
first) pair has a different value for the first one, in that case it
would be clearer to either not do that, or do it explicitly like

case configs of
  first : rest -> postFirst update first >> mapM_ (postStatus update) rest
  [] -> complain about no configs

If you pass a single Bool to a function, it means it can have two
behaviours, which is confusing.  If you pass two Bools, then it can
have 4, which is even more confusing :)  I myself use if/else only
rarely.

Looking a little at Plurkell, 'return =<<' is redundant.  And I'm sure
there's a urlEncode function around so you don't have to build URLs
yourself?

I don't understand the stuff with the words in the case, but it looks
like a confusing way to say 'if word `elem` specialWords'.  There's
also a Set type for that kind of thing.  And that regex stuff is...
quoting?  Isn't there a library function for that too?  It's the sort
of thing a URL library would have.

If not, it's something like 'replace [("|", "%7C"), ("/", "%2F"), ("
", "%20")]', right?  I'm sure there's a replace function like that
floating around somewhere, if not, you can write your own.

And for JSON... wasn't someone just complaining about there being too
many JSON libraries on hackage?  Unless you want to do it yourself for
fun (it's a good way to learn parsing), why not just download one of
those?

That's enough for now, have fun :)



More information about the Haskell-Cafe mailing list