[Haskell-cafe] Advice needed on how to improve some code
Tom Ellis
tom-lists-haskell-cafe-2013 at jaguarpaw.co.uk
Thu Apr 16 08:22:34 UTC 2015
I rather like the >>= invocations. `do` notation would require naming
intermediate variables.
On Wed, Apr 15, 2015 at 11:19:30PM -0400, David Feuer wrote:
> I haven't dug into the guts of this *at all*, but why don't you start by
> using `do` notation instead of a million >>= invocations? It also looks
> like you may have some common patterns you can exploit by defining some
> more functions.
>
> On Wed, Apr 15, 2015 at 8:57 PM, Jeff <jeff at datalinktech.com.au> wrote:
>
> > Hello,
> >
> > I am seeking some advice on how I might improve a bit of code.
> > The function in question reads and parses part of a binary protocol,
> > storing the parsed info as it proceeds.
> >
> > parseDeviceData is called by parseDevice (shown further down).
> >
> > It looks to me like there should be a more concise, less repetitive way to
> > do what
> > parseDeviceData does. Any advice on this would be greatly appreciated.
> >
> > parseDeviceData :: P.Payload -> Parser P.Payload
> > parseDeviceData pl =
> > let
> > mdm = P.dataMask ( P.payloadData pl )
> > in
> > ( let pld = P.payloadData pl in
> > if testBit mdm ( fromEnum D.Sys )
> > then
> > parseDeviceSysData >>=
> > ( \s -> return ( pl { P.payloadData = pld { P.sysData = Just s
> > } } ) )
> > else
> > return pl ) >>=
> > ( \pl' -> let pld = P.payloadData pl' in
> > if testBit mdm ( fromEnum D.GPS )
> > then
> > parseDeviceGPSData >>=
> > ( \s -> return ( pl' { P.payloadData = pld {
> > P.gpsData = Just s } } ) )
> > else
> > return pl' ) >>=
> > ( \pl' -> let pld = P.payloadData pl' in
> > if testBit mdm ( fromEnum D.GSM )
> > then
> > parseDeviceGSMData >>=
> > ( \s -> return ( pl' { P.payloadData = pld {
> > P.gsmData = Just s } } ) )
> > else
> > return pl' ) >>=
> > ( \pl' -> let pld = P.payloadData pl' in
> > if testBit mdm ( fromEnum D.COT )
> > then
> > parseDeviceCOTData >>=
> > ( \s -> return ( pl' { P.payloadData = pld {
> > P.cotData = Just s } } ) )
> > else
> > return pl' ) >>=
> > ( \pl' -> let pld = P.payloadData pl' in
> > if testBit mdm ( fromEnum D.ADC )
> > then
> > parseDeviceADCData >>=
> > ( \s -> return ( pl' { P.payloadData = pld {
> > P.adcData = Just s } } ) )
> > else
> > return pl' ) >>=
> > ( \pl' -> let pld = P.payloadData pl' in
> > if testBit mdm ( fromEnum D.DTT )
> > then
> > parseDeviceDTTData >>=
> > ( \s -> return ( pl' { P.payloadData = pld {
> > P.dttData = Just s } } ) )
> > else
> > return pl' ) >>=
> > ( \pl' -> let pld = P.payloadData pl' in
> > if testBit mdm ( fromEnum D.OneWire )
> > then
> > parseDeviceOneWireData >>=
> > ( \s -> return ( pl' { P.payloadData = pld {
> > P.iwdData = Just s } } ) )
> > else
> > return pl' ) >>=
> > ( \pl' -> if testBit mdm ( fromEnum D.ETD )
> > then
> > parseDeviceEventData pl'
> > else
> > return pl' )
> >
> > The Parser above is a Data.Binary.Strict.Get wrapped in a StateT, where
> > the state is a top-level
> > structure for holding the parsed packet.
> >
> > parseDevice :: Bool -> Parser ()
> > parseDevice _hasEvent =
> > parseTimestamp >>=
> > ( \ts ->
> > if _hasEvent
> > then
> > lift getWord8 >>=
> > ( \e -> lift getWord16be >>=
> > ( \mdm ->
> > return ( P.Payload "" ( Just ts ) $
> > P.blankDevicePayloadData { P.dataMask = mdm
> > , P.eventID = toEnum (
> > fromIntegral e .&. 0x7f )
> > , P.deviceStatusFlag =
> > testBit e 7
> > , P.hasEvent = True
> > } ) ) )
> > else
> > lift getWord16be >>=
> > ( \mdm ->
> > return ( P.Payload "" ( Just ts ) $
> > P.blankDevicePayloadData { P.dataMask = mdm } ) )
> > ) >>=
> > parseDeviceData >>=
> > ( \dpl -> get >>= ( \p -> put ( p { P.payloads = dpl : P.payloads p }
> > ) ) )
> >
> >
> > Here are the data types for the Packet and Payload:
> >
> >
> > data Payload = Payload { imei :: !BS.ByteString
> > , timestamp :: Maybe Word64
> > , payloadData :: PayloadData
> > }
> >
> > data PayloadData = HeartBeatPL
> > | SMSFwdPL { smsMesg :: !BS.ByteString }
> > | SerialPL { auxData :: !Word8
> > , fixFlag :: !Word8
> > , gpsCoord :: !GPSCoord
> > , serialData :: !BS.ByteString
> > }
> > | DevicePL { hasEvent :: !Bool
> > , deviceStatusFlag :: !Bool
> > , eventID :: !E.EventID
> > , dataMask :: !Word16
> > , sysData :: Maybe DS.SysData
> > , gpsData :: Maybe DGP.GPSData
> > , gsmData :: Maybe DGS.GSMData
> > , cotData :: Maybe DC.COTData
> > , adcData :: Maybe DA.ADCData
> > , dttData :: Maybe DD.DTTData
> > , iwdData :: Maybe DO.OneWireData
> > , etdSpd :: Maybe ES.SpeedEvent
> > , etdGeo :: Maybe EG.GeoEvent
> > , etdHealth :: Maybe EH.HealthEvent
> > , etdHarsh :: Maybe EHD.HarshEvent
> > , etdOneWire :: Maybe EO.OneWireEvent
> > , etdADC :: Maybe EA.ADCEvent
> > }
> > deriving ( Show )
> >
> > data Packet = Packet { protocolVersion :: !Word8
> > , packetType :: !PT.PacketType
> > , deviceID :: Maybe BS.ByteString
> > , payloads :: ![ Payload ]
> > , crc :: !Word16
> > }
> > deriving ( Show )
> >
> > Lastly, here is the Parser monad transformer:
> >
> > module G6S.Parser where
> >
> > import Control.Monad.State.Strict
> > import Data.Binary.Strict.Get
> > import qualified Data.ByteString as BS
> >
> > import qualified G6S.Packet as GP
> >
> > type Parser = StateT GP.Packet Get
> >
> > runParser :: Parser a -> BS.ByteString -> Maybe a
> > runParser p bs =
> > let
> > ( result, _ ) = runGet ( runStateT p GP.initPacket ) bs
> > in
> > case result of
> > Right tup -> Just $ fst tup
> > Left _ -> Nothing
> >
> >
> > I hope there is enough info here.
> >
> > Thanks,
> > Jeff
> >
> >
> >
> >
> > _______________________________________________
> > Haskell-Cafe mailing list
> > Haskell-Cafe at haskell.org
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
> >
> _______________________________________________
> Haskell-Cafe mailing list
> Haskell-Cafe at haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
More information about the Haskell-Cafe
mailing list