[Haskell-beginners] Fwd: My first functioning haskell project - a steganography utility

[Brent Yorgey]

On Tue, Jul 13, 2010 at 02:37:17PM +0100, Tim Cowlishaw wrote:
> 
> Hey there all,
> 
> I've just completed my first functional haskell project - a simple utility for steganography - hiding messages within the least significant bit of another sort of data file.
> 
> Therefore, I was wondering if any of you had any pointers about how I could refactor or otherwise improve my code? Any input would be greatly appreciated - whether howling great errors or smaller points of "good haskell style". In particular, I'd be really interested in whether my type declarations are correct - for instance, whether I have been to specific or not specific enough in specifying the types of my functions (Integral vs Int, etc).


- I would write (.&. mask) instead of  (flip (.&.) $ mask).

- decimalNumber is a funny name for a function that interprets a
  binary number. =) Also, I'd write it using a left fold, which is (1)
  nicer than using explicit recursion and (2) more efficient than what
  you have written, since it avoids having to recompute the length of
  the remaining elements and a power of 2 every time.  Like this:

    import Data.List (foldl')

    decimalNumber = foldl' (\n b -> 2*n + b) 0

  Also, note that your call to fromIntegral in decimalNumber is
  unnecessary.

- groupInto is available (as 'chunk') from the 'split' package on Hackage.

- The 'map fromIntegral' applied to (asBits message) seems to be
  unnecessary.  asBits returns a [Word8] and the result you are
  looking for is also [Word8].

- You don't need to nest where clauses like that, all the bindings in
  a where clause can be mutually recursive.  Just put everything in
  the outermost where.  As a matter of fact, your code strikes me as a
  bit where-happy; I would move quite a few of your nested helper
  functions out to the top level.  This makes testing a lot easier.
  You can always choose to not export them from the module if you want
  to hide them.

- binaryDigits seems overly complicated.  How about:

    binaryDigits = reverse . bits
    bits 0 = []
    bits n = (n .&. 1) : bits (n `div` 2)
  
I have a few other suggestions but I'll stop there for now as I should
get back to work. =)  Perhaps I'll send more later if no one else does.

-Brent