Hey guys. Pretty new to haskell here. I've started off by writing a small command-line interface to plurk.. I was just wondering if anyone would be willing to give everything a look-over and lemme know what kinds of things I should be looking to improve upon, style-wise. Not sure I'm currently doing things in the 'haskell-way' so to speak . Thanks a bunch! https://github.com/saiko-chriskun/hermes (note: the JSON and Plurkell submodules are also mine.)
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 :)
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
'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
Before doing a code review I always demand that the author runs over the code with HLint (http://community.haskell.org/~ndm/hlint) - they don't have to necessarily apply all the suggestions, but they do have to at least be aware of obvious alternatives. A code review takes a reasonable amount of time, and it's best to use that for things that machines can't yet figure out - rather than the simpler stuff like the above. Thanks, Neil
On Mon, May 23, 2011 at 12:02 PM, Neil Mitchell
'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
Before doing a code review I always demand that the author runs over the code with HLint (http://community.haskell.org/~ndm/hlint) - they
Very good point. In fact you just inspired me to finally download it and run it on my own code. Thanks for the great tool! While I'm on the topic, I recently wrote a tool that wanted to traverse deep data structures as produced by haskell-src-exts. I wound up with about 50 lines of case expressions and around the time my hands were literally beginning to hurt decided that enough was enough and I should try a generic approach. I heard uniplate was pretty easy to use, and was pretty pleased to turn the entire thing into a single line. It took me a little longer to figure out I needed to use universeBi since all the examples were monotyped, but once I did it Just Worked. Amazing. So thanks again! And maybe you could mention universeBi in the instant introduction?
On Tue, May 24, 2011 at 6:47 AM, Johannes Waldmann
Evan Laforge
writes: [...] a tool that wanted to traverse deep data structures as produced by haskell-src-exts. [...] I heard uniplate was pretty easy to use, and was pretty pleased to turn the entire thing into a single line.
Please post that line here - J.W.
Getting a bit off subject, and it's nothing special: moduleQNames :: Types.Module -> [Types.Qualification] moduleQNames mod = [Types.qualification m | Haskell.Qual _ m _ <- Uniplate.universeBi mod] But just try writing this without a generics library :)
Before doing a code review I always demand that the author runs over the code with HLint (http://community.haskell.org/~ndm/hlint) - they
Very good point. In fact you just inspired me to finally download it and run it on my own code. Thanks for the great tool!
Glad you like it.
While I'm on the topic, I recently wrote a tool that wanted to traverse deep data structures as produced by haskell-src-exts. I wound up with about 50 lines of case expressions and around the time my hands were literally beginning to hurt decided that enough was enough and I should try a generic approach. I heard uniplate was pretty easy to use, and was pretty pleased to turn the entire thing into a single line. It took me a little longer to figure out I needed to use universeBi since all the examples were monotyped, but once I did it Just Worked. Amazing. So thanks again! And maybe you could mention universeBi in the instant introduction?
Yes, I probably should - I'll try and get to that. Of course, I'd also happily accept a patch against http://community.haskell.org/~ndm/darcs/uniplate I use Uniplate inside HLint, and it's invaluable - there are a lot of times when List Comp + universeBi really hits the spot. Thanks, Neil
On Tue, May 24, 2011 at 7:18 PM, Neil Mitchell
Before doing a code review I always demand that the author runs over the code with HLint (http://community.haskell.org/~ndm/hlint) - they
Very good point. In fact you just inspired me to finally download it and run it on my own code. Thanks for the great tool!
Glad you like it.
While I'm on the topic, I recently wrote a tool that wanted to traverse deep data structures as produced by haskell-src-exts. I wound up with about 50 lines of case expressions and around the time my hands were literally beginning to hurt decided that enough was enough and I should try a generic approach. I heard uniplate was pretty easy to use, and was pretty pleased to turn the entire thing into a single line. It took me a little longer to figure out I needed to use universeBi since all the examples were monotyped, but once I did it Just Worked. Amazing. So thanks again! And maybe you could mention universeBi in the instant introduction?
Yes, I probably should - I'll try and get to that. Of course, I'd also happily accept a patch against http://community.haskell.org/~ndm/darcs/uniplate
I use Uniplate inside HLint, and it's invaluable - there are a lot of times when List Comp + universeBi really hits the spot.
+1 on that, I use uniplate for pretty much all my haskell-src-exts tasks these days, works like a charm! I'd love to include some standard traversal functionality in haskell-src-exts that depends on uniplate, but hesitate to do so because of HP aspirations for haskell-src-exts. Neil, what do you reckon the chances of getting uniplate in the HP are? :-) Cheers, /Niklas
Hi Niklas,
I use Uniplate inside HLint, and it's invaluable - there are a lot of times when List Comp + universeBi really hits the spot.
+1 on that, I use uniplate for pretty much all my haskell-src-exts tasks these days, works like a charm! I'd love to include some standard traversal functionality in haskell-src-exts that depends on uniplate, but hesitate to do so because of HP aspirations for haskell-src-exts. Neil, what do you reckon the chances of getting uniplate in the HP are? :-)
I'm happy to have it included. The only likely change is that I currently have basically two versions of all modules (Data.Generics.PlateData and Data.Generics.Uniplate.Data). I'll upload a version 1.7 of Uniplate marking those deprecated, then a 1.8 removing them, and then I'd love to get included in the Platform. Of course, whether it goes in the platform is not really up to me, it's up to the community - but if someone proposes it I'll back it. Thanks, Neil
Evan Laforge schrieb:
I'd keep the line length down to 80 chars, and not use ';'s.
You can find more programming tips in http://www.haskell.org/haskellwiki/Haskell_programming_tips and in other articles of the Haskell Wiki, e.g. in Category:Idioms, Category:Style, Category:FAQ.
participants (7)
-
Alexander Solla -
Chris Bolton -
Evan Laforge -
Henning Thielemann -
Johannes Waldmann -
Neil Mitchell -
Niklas Broberg