<div dir="ltr">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.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 15, 2015 at 8:57 PM, Jeff <span dir="ltr"><<a href="mailto:jeff@datalinktech.com.au" target="_blank">jeff@datalinktech.com.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello,<br>
<br>
I am seeking some advice on how I might improve a bit of code.<br>
The function in question reads and parses part of a binary protocol, storing the parsed info as it proceeds.<br>
<br>
parseDeviceData is called by parseDevice (shown further down).<br>
<br>
It looks to me like there should be a more concise, less repetitive way to do what<br>
parseDeviceData does. Any advice on this would be greatly appreciated.<br>
<br>
parseDeviceData :: P.Payload -> Parser P.Payload<br>
parseDeviceData pl =<br>
let<br>
mdm = P.dataMask ( P.payloadData pl )<br>
in<br>
( let pld = P.payloadData pl in<br>
if testBit mdm ( fromEnum D.Sys )<br>
then<br>
parseDeviceSysData >>=<br>
( \s -> return ( pl { P.payloadData = pld { P.sysData = Just s } } ) )<br>
else<br>
return pl ) >>=<br>
( \pl' -> let pld = P.payloadData pl' in<br>
if testBit mdm ( fromEnum D.GPS )<br>
then<br>
parseDeviceGPSData >>=<br>
( \s -> return ( pl' { P.payloadData = pld { P.gpsData = Just s } } ) )<br>
else<br>
return pl' ) >>=<br>
( \pl' -> let pld = P.payloadData pl' in<br>
if testBit mdm ( fromEnum D.GSM )<br>
then<br>
parseDeviceGSMData >>=<br>
( \s -> return ( pl' { P.payloadData = pld { P.gsmData = Just s } } ) )<br>
else<br>
return pl' ) >>=<br>
( \pl' -> let pld = P.payloadData pl' in<br>
if testBit mdm ( fromEnum D.COT )<br>
then<br>
parseDeviceCOTData >>=<br>
( \s -> return ( pl' { P.payloadData = pld { P.cotData = Just s } } ) )<br>
else<br>
return pl' ) >>=<br>
( \pl' -> let pld = P.payloadData pl' in<br>
if testBit mdm ( fromEnum D.ADC )<br>
then<br>
parseDeviceADCData >>=<br>
( \s -> return ( pl' { P.payloadData = pld { P.adcData = Just s } } ) )<br>
else<br>
return pl' ) >>=<br>
( \pl' -> let pld = P.payloadData pl' in<br>
if testBit mdm ( fromEnum D.DTT )<br>
then<br>
parseDeviceDTTData >>=<br>
( \s -> return ( pl' { P.payloadData = pld { P.dttData = Just s } } ) )<br>
else<br>
return pl' ) >>=<br>
( \pl' -> let pld = P.payloadData pl' in<br>
if testBit mdm ( fromEnum D.OneWire )<br>
then<br>
parseDeviceOneWireData >>=<br>
( \s -> return ( pl' { P.payloadData = pld { P.iwdData = Just s } } ) )<br>
else<br>
return pl' ) >>=<br>
( \pl' -> if testBit mdm ( fromEnum D.ETD )<br>
then<br>
parseDeviceEventData pl'<br>
else<br>
return pl' )<br>
<br>
The Parser above is a Data.Binary.Strict.Get wrapped in a StateT, where the state is a top-level<br>
structure for holding the parsed packet.<br>
<br>
parseDevice :: Bool -> Parser ()<br>
parseDevice _hasEvent =<br>
parseTimestamp >>=<br>
( \ts -><br>
if _hasEvent<br>
then<br>
lift getWord8 >>=<br>
( \e -> lift getWord16be >>=<br>
( \mdm -><br>
return ( P.Payload "" ( Just ts ) $<br>
P.blankDevicePayloadData { P.dataMask = mdm<br>
, P.eventID = toEnum ( fromIntegral e .&. 0x7f )<br>
, P.deviceStatusFlag = testBit e 7<br>
, P.hasEvent = True<br>
} ) ) )<br>
else<br>
lift getWord16be >>=<br>
( \mdm -><br>
return ( P.Payload "" ( Just ts ) $<br>
P.blankDevicePayloadData { P.dataMask = mdm } ) )<br>
) >>=<br>
parseDeviceData >>=<br>
( \dpl -> get >>= ( \p -> put ( p { P.payloads = dpl : P.payloads p } ) ) )<br>
<br>
<br>
Here are the data types for the Packet and Payload:<br>
<br>
<br>
data Payload = Payload { imei :: !BS.ByteString<br>
, timestamp :: Maybe Word64<br>
, payloadData :: PayloadData<br>
}<br>
<br>
data PayloadData = HeartBeatPL<br>
| SMSFwdPL { smsMesg :: !BS.ByteString }<br>
| SerialPL { auxData :: !Word8<br>
, fixFlag :: !Word8<br>
, gpsCoord :: !GPSCoord<br>
, serialData :: !BS.ByteString<br>
}<br>
| DevicePL { hasEvent :: !Bool<br>
, deviceStatusFlag :: !Bool<br>
, eventID :: !E.EventID<br>
, dataMask :: !Word16<br>
, sysData :: Maybe DS.SysData<br>
, gpsData :: Maybe DGP.GPSData<br>
, gsmData :: Maybe DGS.GSMData<br>
, cotData :: Maybe DC.COTData<br>
, adcData :: Maybe DA.ADCData<br>
, dttData :: Maybe DD.DTTData<br>
, iwdData :: Maybe DO.OneWireData<br>
, etdSpd :: Maybe ES.SpeedEvent<br>
, etdGeo :: Maybe EG.GeoEvent<br>
, etdHealth :: Maybe EH.HealthEvent<br>
, etdHarsh :: Maybe EHD.HarshEvent<br>
, etdOneWire :: Maybe EO.OneWireEvent<br>
, etdADC :: Maybe EA.ADCEvent<br>
}<br>
deriving ( Show )<br>
<br>
data Packet = Packet { protocolVersion :: !Word8<br>
, packetType :: !PT.PacketType<br>
, deviceID :: Maybe BS.ByteString<br>
, payloads :: ![ Payload ]<br>
, crc :: !Word16<br>
}<br>
deriving ( Show )<br>
<br>
Lastly, here is the Parser monad transformer:<br>
<br>
module G6S.Parser where<br>
<br>
import Control.Monad.State.Strict<br>
import Data.Binary.Strict.Get<br>
import qualified Data.ByteString as BS<br>
<br>
import qualified G6S.Packet as GP<br>
<br>
type Parser = StateT GP.Packet Get<br>
<br>
runParser :: Parser a -> BS.ByteString -> Maybe a<br>
runParser p bs =<br>
let<br>
( result, _ ) = runGet ( runStateT p GP.initPacket ) bs<br>
in<br>
case result of<br>
Right tup -> Just $ fst tup<br>
Left _ -> Nothing<br>
<br>
<br>
I hope there is enough info here.<br>
<br>
Thanks,<br>
Jeff<br>
<br>
<br>
<br>
<br>
_______________________________________________<br>
Haskell-Cafe mailing list<br>
<a href="mailto:Haskell-Cafe@haskell.org">Haskell-Cafe@haskell.org</a><br>
<a href="http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe" target="_blank">http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe</a><br>
</blockquote></div><br></div>