[Haskell-cafe] code review?

[Alexander Solla]

Good points.  Here are a few more.  Use more explicit types.  You can cut
down on if-then-else's by using pattern matching and guards.  (For example,
the first if-then-else in postStatus can be turned into:

newtype PostToAllNetworks = PostToAll Bool
newtype FirstPost = FirstPost Bool

postToAllNetworks :: PostToAllNetworks -> FirstPost -> IO Bool
PostToAllNetworks (PostToAll True) (First True) = do
    putStr $ "Post to all networks? [y/n] "
    hSetBuffering stdin NoBuffering ; hFlush stdout
    postAll <- getChar ; hSetBuffering stdin LineBuffering
    return (postAll' == 'y')

postToAllNetworks _ _ = return False

Notice I'm not just playing golf here.  This is easier to read.  Also, keep
in mind that "post" is a noun and a verb.  It is very "functional" to write
functions whose names are nouns.  It might be nice if Haskell had some
syntax like Ruby does, so we could ask "postToAllNetworks?"  But since we
don't, you should be open to the possibility that a name can be either.

On Sun, May 22, 2011 at 3:32 PM, Evan Laforge <qdunkan at gmail.com> wrote:

> 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 :)
>
> _______________________________________________
> Haskell-Cafe mailing list
> Haskell-Cafe at haskell.org
> http://www.haskell.org/mailman/listinfo/haskell-cafe
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/haskell-cafe/attachments/20110522/03bc7df8/attachment.htm>