On 16/04/15 04:19, 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 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.

Lens[0] might help abstract the common pattern of nested record updates. You should be able to get it into something that looks more like this: whenBit flag parser setter pld | view dataMask pld `testBit` fromEnum flag = do s <- parser return $ set setter (Just s) pld | otherwise = return pld parseDevicePayloadData = foldr (>=>) return [ whenBit Sys parseDeviceSysData sysData , whenBit GPS parseDeviceGPSData gpsData ... ] [0] http://hackage.haskell.org/package/lens Claude

...

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

-- http://mathr.co.uk

On 16 Apr 2015, at 09:22, Tom Ellis wrote:

I rather like the >>= invocations. `do` notation would require naming intermediate variables.

But the >>= requires such intermediate variables anyway: all the pl' after the \

...

On Wed, Apr 15, 2015 at 8:57 PM, Jeff wrote:

...

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' ) >>=

Do notation is just syntax sugar for the above do pl' <- ..... pl' <- .... pl' <- ... No additional variable invention required ! Andrew Butterfield School of Computer Science & Statistics Trinity College Dublin 2, Ireland

Oh sorry, I now see you were talking (at least) about other uses of >>= where do notation would be very helpful. On Thu, Apr 16, 2015 at 09:22:34AM +0100, Tom Ellis wrote:

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 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@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe

...

_______________________________________________ Haskell-Cafe mailing list Haskell-Cafe@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe

_______________________________________________ Haskell-Cafe mailing list Haskell-Cafe@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe

Thanks Tom, David and Claude for your replies.

On 16 Apr 2015, at 4:03 pm, Tom Ellis wrote:

The first thing you should do is define

parseDeviceGPSDataOf constructor parser setField = ( \pl' -> let pld = P.payloadData pl' in if testBit mdm ( fromEnum constructor ) then parser >>= ( \s -> return ( pl' { P.payloadData = setField pld (Just s) } } ) ) else return pl' )

and your chain of binds will become

setgpsData pld = pld { P.gpsData = Just s } ...

parseDeviceDataOf D.GPS parseDeviceGPSData setgpsData >>= parseDeviceDataOf D.GSM parseDeviceDSMData setgsmData >>= parseDeviceDataOf D.COT parseDeviceCOTData setcotData >>= ...

Then I would probably write

deviceSpecs = [ (D.GPS, parseDeviceGPSData, setgpsData) , (D.GSM, parseDeviceDSMData, setgsmData) , (D.COT, parseDeviceCOTData, setcotData) ]

and turn the chain of binds into a fold.

I’ll do as you have suggested Tom. Thanks. Jeff

Thanks Sylvain, I had a suspicion that Applicative might be applicable ( ;-) ) I like Tom Ellis’ suggestions but I might try this out. Jeff

On 16 Apr 2015, at 6:24 pm, Sylvain Henry wrote:

I don't think you need to update record fields of a blank record: you can create the record using applicative operators instead. Something like:

parseDevicePLData :: Bool -> Get PayloadData parseDevicePLData hasEv = do rawEvId <- if hasEv then getWord8 else return 0 -- I guessed the 0 value let evId = toEnum (fromIntegral rawEvId .&. 0x7f) let statusFlag = testBit rawEvId 7 mask <- getWord16be

let parseMaybe e p = if testBit mask (fromEnum e) then Just <$> p else return Nothing

DevicePL hasEv statusFlag evId mask <$> parseMaybe D.GPS parseDeviceGPSData <*> parseMaybe D.GSM parseDeviceGSMData <*> parseMaybe D.COT parseDeviceCotData <*> ...

parseDevicePL :: Bool -> Get Payload parseDevicePL hasEv = do ts <- parseTimestamp P.Payload "" (Just ts) <$> parseDevicePLData hasEv

Then you can lift these "parsers" into you Parser monad only when you need it.

-- Sylvain

2015-04-16 8:37 GMT+02:00 Jeff :

...

Thanks Tom, David and Claude for your replies.

...

On 16 Apr 2015, at 4:03 pm, Tom Ellis wrote:

The first thing you should do is define

parseDeviceGPSDataOf constructor parser setField = ( \pl' -> let pld = P.payloadData pl' in if testBit mdm ( fromEnum constructor ) then parser >>= ( \s -> return ( pl' { P.payloadData = setField pld (Just s) } } ) ) else return pl' )

and your chain of binds will become

setgpsData pld = pld { P.gpsData = Just s } ...

parseDeviceDataOf D.GPS parseDeviceGPSData setgpsData >>= parseDeviceDataOf D.GSM parseDeviceDSMData setgsmData >>= parseDeviceDataOf D.COT parseDeviceCOTData setcotData >>= ...

Then I would probably write

deviceSpecs = [ (D.GPS, parseDeviceGPSData, setgpsData) , (D.GSM, parseDeviceDSMData, setgsmData) , (D.COT, parseDeviceCOTData, setcotData) ]

and turn the chain of binds into a fold.

I’ll do as you have suggested Tom. Thanks.

Jeff

_______________________________________________ Haskell-Cafe mailing list Haskell-Cafe@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe