This is the perfect kind of question to post to the mailing list!
I will go down the two programs and make minor comments and then review
their overall structure.
-- encode.hs
* Delete `runEffect`. It's not doing anything. The reason that it
still type-checked was because your base monad was polymorphic over
`MonadIO`, so it let you accidentally insert an additional `Pipe` layer
(which was not doing anything). As a side note, I think I made a
mistake by parametrizing the `Pipes.Prelude` utilities over `MonadIO` (I
prefer using `hoist` now), but I don't want to make a breaking change to
fix it.
* Good use of `withFile` instead of `pipes-safe`. I feel like too many
people unnecessarily use `pipes-safe` when `withFile` suffices.
* Use `view Pipes.ByteString.pack p` instead of `p >-> PP.map
B.singleton`. It will group your Word8's into a more efficient chunk
size. Your current formulation will call a separate write command for
every single byte, which is very inefficient.
* For the reverse direction (i.e. `bytes`), you can either:
A) Use `view (from Pipes.ByteString.pack)`, but that requires a `lens`
dependency (which I think is not good). I plan to fix that by providing
an `unpack` lens in an upcoming `pipes-bytestring` release. I created
an issue for this:
https://github.com/Gabriel439/Haskell-Pipes-ByteString-Library/issues/36
B) Use `mapFoldable`:
bytes = Pipes.Prelude.mapFoldable BS.unpack
That's much more efficient. The problem with your `bytes` function is
that it uses `foldl`, which triggers a bunch of left-associated binds,
generating quadratic time complexity in the number of bytes:
((((return ()) >> yield byte1) >> yield byte2) >> yield byte3
`mapFoldable`, on the other hand, is implemented in terms of `each`,
which uses a right-fold like this:
each = Data.Foldable.foldr (\a p -> yield a >> p) (return ())
... which triggers build/fold fusion and also gives linear time complexity:
yield byte1 >> (yield byte2 >> (yield byte3 >> return ()))
* If you're willing to skip the error message, you can shorten
`encodeByte` to:
encodeByte t = for cat $ \b -> each(b `M.lookup` t)
... which is the same thing as:
encodeByte t = Pipes.Prelude.mapFoldable (`M.lookup` t)
* I should probably provide a function that transforms `Parser`s to
functions between `Producer`s to simplify your `dirsBytes` code. I also
find myself writing that same pattern way too many times. I just
created an issue to remind myself to do this:
https://github.com/Gabriel439/Haskell-Pipes-Parse-Library/issues/28
-- decode.hs
* Is there any reason why you `drain` unused input using `limit` instead
of just using `take` by itself?
* Same thing as `encode`.hs: try using `Pipes.ByteString.pack` and
`Pipes.Prelude.mapFoldable Data.ByteString.unpack` for much greater
efficiency translating between `Word8`s and `ByteString`s
* You can make the code for `searchPT` more reusable by first defining a
`Consumer'` (note the prime!) that produces a single `Direction`, like this:
searchPT :: forall m. Monad m => PreTree Word8 -> Consumer'
Direction m Word8
searchPT pt0 = go pt0
where
go :: PreTree Word8 -> Consumer Direction m Word8
go (PTLeaf x ) = return x
go (PTNode pt1 pt2) = do
dir <- await
go $ case dir of
DLeft -> pt1
DRight -> pt2
... and then you can optionally upgrade that to a `Pipe` like this:
searchPT pt >~ cat:: Pipe Direction Word8 m r
That decouples the logic for parsing one direction from the logic for
looping.
* Also, there's nothing `Word8`-specific about your `searchPT`
function. Consider generalizing the type to any value.
* You can simplify the implementation of `dirs` using `mapFoldable`:
dirs = Pipes.Prelude.mapFoldable byteToDirs
Overall the architecture of your program looks correct. I don't see any
obvious non-idiomatic things that you are doing.
On 5/2/14, 2:43 AM, Justin Le wrote:
Hi pipes people;
I really don't know too much about pipes, but an entire section in a
project tutorial I am writing is going to be dedicated to hooking up
all of the pipes plumbing together. Seeing as this might also be
possibly used as a pipes tutorial, I just wanted to make sure that my
pipes code is idiomatic/not awful/not going to set back your progress
by generations. Does anyone mind maybe giving it a quick look over?
:) I would really appreciate it, and credit will be given where
deserved :) I hope it is not too imposing for me to ask!
It's actually a pair of programs --- a Huffman compression encoder and
decoder.
The
encoder: https://github.com/mstksg/inCode/blob/master/code-samples/huffman/encode.hs
The
decoer: https://github.com/mstksg/inCode/blob/master/code-samples/huffman/decode.hs
I tried my best to abstract away the actual mechanisms of the huffman
logic where I could; it does peak in at some times, but the comments
should give you a general high-level idea of what each function is
trying to do. For reference, the series itself explaining the logic
is hosted at http://blog.jle.im/entries/series/+huffman-compression
I am pretty sure that the code gives away my unfamiliarity :)
Thank you all!
Justin
--
You received this message because you are subscribed to the Google
Groups "Haskell Pipes" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected]
<mailto:[email protected]>.
To post to this group, send email to [email protected]
<mailto:[email protected]>.
--
You received this message because you are subscribed to the Google Groups "Haskell
Pipes" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
