[Haskell-cafe] PrefixMap: code review request

[Robert Dockins]

On Feb 27, 2006, at 2:30 PM, David F.Place wrote:

> Hi,
>
> I'm a newish Haskell hacker with way too much experience hacking  
> Lisp.    At first, I found my Haskell code looking very lisp-y.   I  
> think my code is becoming more idiomatic haskell.  I would be very  
> grateful to anyone who would take a glance at the code below and  
> point out any un-idiomatic usages that jump out.  It's a small  
> module from a program which looks for palindromes in a list of  
> words.  Thanks very much.

[snip]

> partList :: Ord k => [([k],v)]->[k]->[(k,[([k],v)])]
> partList pairs alphabet = reverse . fst $ foldl' f ([],pairs) alphabet
>     where f (result,pairs) l = (result',rest)
>               where (part,rest) = span ((==l) . head . fst) pairs
> 		    result' = if null part
> 			         then result
> 			         else (l,part):result

I don't think I've ever seen nested "where"s before ;-)  I'd probably  
avoid that; it's really hard to read.  If your function is  
sufficiently complicated that it needs its own "where" clause, you  
should probably just promote it to the top level.  If it is truly  
internal, you can avoid exporting it with the module export list.

[snip]

 > searchMap :: Ord k => (v -> vv) -> [k] -> PrefixMap k v -> [vv]

Humm... double "v" seems like a pretty poor choice for a type  
variable name.

[snip]


Just a couple of general comments:

-- you don't seem to like horizontal whitespace much.  I know, I  
know, whitespace placement can be a highly personal thing, but I find  
most haskellers usually use a bit more horizontal whitespace,  
particularly in function signatures.  The arrow is almost always  
written with surrounding spaces.  I personally like space after  
commas in tuples and lists.  Several of your list comprehensions  
would also be easier to read with a bit of whitespace.  I also tend  
to like '=' signs lined up in a column for lets, pattern function  
definitions and wheres.

-- Nested tuple and lists types are pretty hard to read.  In your  
code [([k],v)] appears a lot.  Consider defining a type alias for it.



Rob Dockins

Speak softly and drive a Sherman tank.
Laugh hard; it's a long way to the bank.
           -- TMBG