[Haskell-cafe] Advice needed on how to improve some code

David Feuer david.feuer at gmail.com
Thu Apr 16 03:19:30 UTC 2015


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/haskell-cafe/attachments/20150415/c2599071/attachment-0001.html>


More information about the Haskell-Cafe mailing list