Advice needed on how to improve some code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Advice needed on how to improve some code

Jeff
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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

David Feuer
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 <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe


_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Claude Heiland-Allen-3
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 <[hidden email]> 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

_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Tom Ellis
In reply to this post by Jeff
On Thu, Apr 16, 2015 at 10:57:41AM +1000, Jeff wrote:

>       ( \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 } } ) )

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.

Tom
_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Jeff
Thanks Tom, David and Claude for your replies.



> On 16 Apr 2015, at 4:03 pm, Tom Ellis <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Tom Ellis
In reply to this post by David Feuer
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 <[hidden email]> 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
> > [hidden email]
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
> >

> _______________________________________________
> Haskell-Cafe mailing list
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe

_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Sylvain HENRY
In reply to this post by Jeff
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 <[hidden email]>:

> Thanks Tom, David and Claude for your replies.
>
>
>
>> On 16 Apr 2015, at 4:03 pm, Tom Ellis <[hidden email]> 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
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Andrew Butterfield-2
In reply to this post by Tom Ellis

> On 16 Apr 2015, at 09:22, Tom Ellis <[hidden email]> 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 <[hidden email]> 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

_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Tom Ellis
In reply to this post by Tom Ellis
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 <[hidden email]> 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
> > > [hidden email]
> > > http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
> > >
>
> > _______________________________________________
> > Haskell-Cafe mailing list
> > [hidden email]
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
>
> _______________________________________________
> Haskell-Cafe mailing list
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Tom Ellis
In reply to this post by Andrew Butterfield-2
On Thu, Apr 16, 2015 at 09:32:45AM +0100, Andrew Butterfield wrote:
>
> > On 16 Apr 2015, at 09:22, Tom Ellis <[hidden email]> 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 \  

Not if the pl' doesn't exist because it became part of the body of an
abstracted function.  Anyway, I now suspect David Feuer was speaking about
use of >>= elsewhere in Jeff's code.


> >> On Wed, Apr 15, 2015 at 8:57 PM, Jeff <[hidden email]> 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' ) >>=
_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe
Reply | Threaded
Open this post in threaded view
|

Re: Advice needed on how to improve some code

Jeff
In reply to this post by Sylvain HENRY
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 <[hidden email]> 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 <[hidden email]>:
>> Thanks Tom, David and Claude for your replies.
>>
>>
>>
>>> On 16 Apr 2015, at 4:03 pm, Tom Ellis <[hidden email]> 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
>> [hidden email]
>> http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe

_______________________________________________
Haskell-Cafe mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/haskell-cafe