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
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@haskell.org http://www.haskell.org/mailman/listinfo/haskell-cafe