Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm writing a Telnet parser in Haskell that can run within any monad that implements some event-handling functions. When it finishes processing a block of input - whether because it read it all or because it was interrupted by an event handler - it returns the remainder of the text and a continuation for more input.

Right now, the code works, but it's pretty ugly. I've tried to make the patterns I saw in the program as obvious as possible, but I'm stymied at the actual refactoring stage.

I'm still pretty new to Haskell, so any feedback and advice on best practices is appreciated!

import qualified Data.ByteString as BS
import           Data.ByteString (ByteString)
import Data.Word (Word8)
import Control.Monad

 - I suspect I can change these datatypes to use something like
 -   data Parser m r = Parser (ByteString -> m r)
 - but I'm not sure how to go about doing that.
data TelnetParser m = TelnetParser (ByteString -> m (TelnetState m))
type TelnetState m = (ByteString, TelnetParser m)

 - These are the handler functions that the parser requires.
 - These allow you to execute other monadic actions in response to
 - Telnet events.
class (Monad m) => Telnet m where
  onText    :: Word8 ->          m ()
  onCommand :: Word8 ->          m Bool
  onOption  :: Word8 -> Word8 -> m Bool

  onText _     = return ()
  onCommand _  = return True
  onOption _ _ = return True

readTelnet :: (Telnet m) => ByteString -> TelnetParser m -> m (TelnetState m)
readTelnet str (TelnetParser f)
  | BS.null str = return (str, TelnetParser f)
  | otherwise   = f str

isOption :: Word8 -> Bool
isOption = (`elem` [250..254])

decideContinue :: (Telnet m) => Bool -> ByteString -> TelnetParser m -> m (TelnetState m)
decideContinue True = readTelnet
decideContinue False = curry return

 - These functions implement the various states of the Telnet parser.

readOption :: (Telnet m) => Word8 -> ByteString -> m (TelnetState m)
readOption cmd str = do
    action <- decideContinue `liftM` onOption cmd ch
    action rest (TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

readIAC :: (Telnet m) => ByteString -> m (TelnetState m)
readIAC str
    | ch == 255    = onText ch >> (rest `readTelnet` TelnetParser readText)
    | isOption ch  =              (rest `readTelnet` TelnetParser (readOption ch))
    | otherwise    = do
        action <- decideContinue `liftM` onCommand ch
        action rest (TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

readCR :: (Telnet m) => ByteString -> m (TelnetState m)
readCR str
    | ch == 10    = onText 13 >> onText 10 >> (rest `readTelnet` TelnetParser readText)
    | ch ==  0    = onText 13 >>              (rest `readTelnet` TelnetParser readText)
    | otherwise   = onText 13 >>              (rest `readTelnet` TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

readText :: (Telnet m) => ByteString -> m (TelnetState m)
readText str
    | ch == 255   =              (rest `readTelnet` TelnetParser readIAC)
    | ch == 13    =              (rest `readTelnet` TelnetParser readCR)
    | otherwise   = onText ch >> (rest `readTelnet` TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)
share|improve this question

I think one of the reason why functional programming is usually easier is its natural modularity. Consider to design your solution in a more layered way. I.e. separate parser and invocations. Avoid callbacks in favor of lazy stream of events (see unsafeInterleaveIO) or borrow ideas from reactive frameworks. Also see Data.Binary.Get module or parsec package for implementing parser.

share|improve this answer

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.