[Haskell-cafe] PrefixMap: code review request

[Brian Hulley]

David F.Place wrote:
[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

If the above code is put into a non-literate file as:

 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

there is a parse error (using ghc) at the line beginning with result'. This 
binding doesn't line up with anything. Also the second 'where' is 
dangerously close to the column started by the 'f' after the first 'where' 
(may not be noticeable in this email due to whatever font it is being 
displayed in but it's only indented by one space) which makes it a bit 
tricky to read.

I suggest:

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

or:

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

because always starting an 'if' construct on a new line ensures that you 
will never ever have any problems with it's layout (especially helpful for 
people used to C) when you use it in a 'do' block.

Also, both the above variants ensure that your code can be edited using 
variable width fonts, any tabbing regime you like (as long as leading 
whitespace only has tabs and never spaces), and will be immune to identifier 
renamings. The golden rule for 'safe' layout is always to start a layout 
block on the next line after 'do' 'where' 'of' 'let' and to try to resist 
the tempation to save a line by squashing the first binding onto the same 
line as the 'let' etc.

The second variant has the added advantage that the horizontal indentation 
is kept under control (eg in the above all indenting is 3 spaces further in) 
whereas when people write things like

if p == q then let a = 4
                          b = if a ==4 then let q = 2
                                                         s = 56

after only 3 indents the code is already half way across the screen (not to 
mention the fact that the above layout is completely brittle and can be 
destroyed by a simple search-and-replace op to change 'q' to 'hello')

Of course all the above is just easy-to-talk-about technicalities of layout 
and you were really interested in getting feedback about idiomatic usage - 
hopefully someone else will give some feedback about that (I'm too lazy...) 
:-)

Regards, Brian.