Factoring out common code in two methods of a class

jvector has asked for the wisdom of the Perl Monks concerning the following question:

Monks,

I have two complementary methods in a class. They pack a hash of values into a binary scalar and unpack it. One uses a format declaration to control the pack operation, the other uses a format declaration to control the unpack. The two subs "want" the same format declaration but so far I am having to declare it twice which has a nasty smell to it. In fact during my development so far I have already been bitten by inconsistent changes in the two declarations.

Update: further reading of the docs tells me that pack/unpack is not the way to do the bit-level twiddling. So I am looking into vec for the flag handling. But I suspect pack is still required for the other parts of the message structure, and the general query about redundant code still applies.

I am stuck trying to think of a way to make the redundant duplicate declarations into a single one that is accessible to both subs.

The code is below - I put in the whole of the two subs in order to avoid over-simplifying, as they are not 'too large'. Normal strictures and warnings apply. (I did have to edit after the copy/paste to remove trailing whitespace. Other than that, WhatYouSeeIsWhatIGot.)

#########
sub new {
#########
  my $class = shift;
  my $self = {}; 
  bless ($self,$class);  

  my %fragArgs = %{ $_[0] };            # dereference the hashref of m
+essage fragment parameters

  die "no message ID!"    unless defined $fragArgs{msgID}   ;
  die "no frag seq!"      unless defined $fragArgs{fragSeq} ;
  die "no morefrag flag!" unless defined $fragArgs{moreFrags} ;
  die "no frag data !"    unless defined fragArgs{fragData} ;
  &setUpLogging;  

  # Declare the constant field lengths for the pack statement  
  # to package the msg data into a fragment
  my $fmtMsgID             = 'H4';# 16 bits     Msg ID 4 hex digits
  my $fmtFragSeq           = 'b7';# 7 bits      Frag Seq
  my $fmtMoreFrags         = 'b'; # 1 bit       More Frags flag       
+                                            
  my $fmtFlagUnitIDPresent = 'b'; # 1 bit       Unit ID Present 
  my $fmtFlagCRCPresent    = 'b'; # 1 bit       CRC Present 
  my $fmtFlagsUnused       = 'b6'; # 6 bits     Unused                
+                                      
  my $fmtfragLen           = 'H2'; # 8 bits     Frag Length           
+                                    
  #my $fmtFragData         is 'variable';                   
  my $fmtUnitID            = 'A8'; # if present, unit ID is 8 chars
  my $fmtFragCRC           = 'H4'; # if present, CRC is a 16 bit word 
+  

  # create the fragment packing format according to the data length an
+d flags           
  my $fragFormat =
    $fmtMsgID .
      $fmtFragSeq .
        $fmtMoreFrags .
          $fmtFlagUnitIDPresent .
            $fmtFlagCRCPresent .
              $fmtFlagsUnused .
                $fmtfragLen .
                  'A' . (length $fragArgs{fragData} ) .
                    ($fragArgs{unitID} ? $fmtUnitID : '') .
                      ($fragArgs{doCRC} ? $fmtFragCRC: '') ;

  #pack the individual bits of the first message hash according to the
+ pack-format. 
  # include Unit ID and/or CRC data if they are declared for this mess
+age. 

  DEBUG "packing message frag using format $fragFormat";
  my $wrappedFrag = pack $fragFormat,
    $fragArgs{msgID},
      $fragArgs{fragSeq},
        $fragArgs{moreFrags},
          ($fragArgs{unitID}? 1 : 0 ),     # set UnitIDPresent bit if 
+given                
            ($fragArgs{CRC} ? 1 : 0 ) ,    # set CRCPresent bit if giv
+en
              0,                             # 0 for the unused bits  
+      
                length $fragArgs{fragData},
                  $fragArgs{fragData},
                    ($fragArgs{unitID}  ?  $fragArgs{unitID} :''),    
+# include unit ID if it was given     
                      ($fragArgs{doCRC} ?  &doCRC($fragArgs{fragData})
+ :'');

  $Data::Dumper::Sortkeys = $Data::Dumper::Terse = $Data::Dumper::Inde
+nt = 1;
  print Dumper(\%fragArgs) . "gives:\n".  Dumper($wrappedFrag);

  return $wrappedFrag;
}

#############  
sub unwrap {
#############
#given a msg fragment in packed binary return a message hash         
  my $class = shift;
  my $self = {};
  bless ($self,$class);

  my $wrappedFrag = $_[0] ;
  my %msgFrag = ();

  die "no message frag!"    unless defined $wrappedFrag;

  &setUpLogging;

  # Declare the constant field lengths for the unpack statement
  # to extract the msg data from a fragment 
  my $fmtMsgID             = 'H4';# 16 bits     Msg ID                
+                          
  my $fmtFragSeq           = 'b7';# 7 bits      Frag Seq              
+                           
  my $fmtMoreFrags         = 'b'; # 1 bit       More Frags flag       
+                                 
  my $fmtFlagUnitIDPresent = 'b'; # 1 bit       Unit ID Present       
+                              
  my $fmtFlagCRCPresent    = 'b'; # 1 bit       CRC Present           
+                          
  my $fmtFlagsUnused       = 'b6'; # 6 bits Unused
  my $fmtfragLen           = 'H2'; # 8 bits     Frag Length           
+                                   
  #my $fmtFragData         is 'variable';                   
  my $fmtUnitID            = 'A8'; # if present, unit ID is 8 chars
  my $fmtFragCRC           = 'H4'; # if present, CRC is a 16 bit word 
+  

  # We will have to take the frag apart in a number of bites.        

  # 1. Msg ID, Frag Seq, More Frags flag, Unit ID Present, CRC Present
+, Frag Length.           
  my $fragFormat =
    $fmtMsgID .
      $fmtFragSeq .
        $fmtMoreFrags .
          $fmtFlagUnitIDPresent .
            $fmtFlagCRCPresent .
              $fmtFlagsUnused .
                $fmtfragLen ;
 DEBUG "unpacking message frag using format $fragFormat";
  ($msgFrag{msgID},
   $msgFrag{fragSeq},
   $msgFrag{moreFrags},
   $msgFrag{unitID},
   $msgFrag{CRC},
   my $junk,                             #for the unused flagbits
   my $fragLength)
    = unpack $fragFormat, $wrappedFrag;

  $Data::Dumper::Sortkeys = $Data::Dumper::Terse =  $Data::Dumper::Ind
+ent = 1;
  print "unpack got \n" . Dumper(\%msgFrag) . "from:\n". Dumper($wrapp
+edFrag);

  # further code to be added ...
  return %msgFrag;
}
[download]

The pack format definition is deliberately built up from individual pieces because (i) formats vary from one message to another; (ii) the overall message-format design itself may be subject to change.

Clearly common side-effect-free actions can be subroutined out, as with &setUpLogging; (aside: I feel sheepish using the "&" form in present company, but I still find it helpful - if anyone proides a link to a 'use of "&" harmful' node I promise to go read it.)

But I do not see how I can do the same thing with all those my $fmtFoo's.

The more I strive to do things "the right way", the more there is to learn. Please help to put me back on The Path.

This signature will be better than the next one

Comment on Factoring out common code in two methods of a class Select or Download Code

Replies are listed 'Best First'.
Re: Factoring out common code in two methods of a class by BrowserUk (Patriarch) on Aug 05, 2009 at 17:59 UTC
That is a very cumbersome way to do things, as well as being a performance penalty rebuilding the template every time you use it. The basic problem is that you are setting up a (series of) variable(s), when you should be using a constant! Yes, the template might need to change in the future, but it won't be changing at runtime, so it is a constant. Both sets of lines can be replaced by a single line (and a comment): `use constant PACK_TEMPLATE = 'H4 b7 b1 b1 b1 b6 H2 A8 H4'; =comment 'H4';# 16 bits Msg ID 'b7';# 7 bits Frag Seq 'b'; # 1 bit More Frags flag 'b'; # 1 bit Unit ID Present 'b'; # 1 bit CRC Present 'b6'; # 6 bits Unused 'H2'; # 8 bits Frag Length 'A8'; # if present, unit ID is 8 chars 'H4'; # if present, CRC is a 16 bit word =cut` [download] Now, you just use `PACK_TEMPLATE` (give it a better name appropriate to your application), whereever you need it. It will be private to the package/class you declare it in, so there are no action at distance concerns. Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error. "Science is about questioning the status quo. Questioning authority". In the absence of evidence, opinion is indistinguishable from prejudice. RIP PCW	[reply] [d/l] [select]
Re^2: Factoring out common code in two methods of a class by jvector (Friar) on Aug 05, 2009 at 18:48 UTC
Thanks for that. I had wondered about "`use constant...`" but had it in my head that I would need a constant for each 'field' and been put off by the anticipated verbosity of it. But I had been blinkered about putting them all into one constant. I guess I'll have to leave off the last two parts which in this application are optional, and add them to PACK_TEMPLATE programatically after the length field ... hmm, that's OK. And yes, you're right that there is a performance implication on constructing that format string each time. Not a lot, but every little bit hurts... The next signature will be better than this one	[reply] [d/l]
Re^3: Factoring out common code in two methods of a class by BrowserUk (Patriarch) on Aug 06, 2009 at 00:07 UTC
I guess I'll have to leave off the last two parts which in this application are optional, and add them to PACK_TEMPLATE programatically after the length field Personally, I'd just create 2 or 3 constants and select between them at runtime: `use constant T1 = '...'; use constant T2 = t1 . '...'; use constant T3 = T1 . '...'; # or use constant T3 = T2 . '...'; ... my $packed = pack ( T1, T2, T3 )[ condition() ], @bits;` [download] Wher `condition()` returns an integer 0 .. 2. Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error. "Science is about questioning the status quo. Questioning authority". In the absence of evidence, opinion is indistinguishable from prejudice. RIP PCW	[reply] [d/l] [select]
Re^3: Factoring out common code in two methods of a class by metaperl (Curate) on Aug 05, 2009 at 19:42 UTC
I guess I'll have to leave off the last two parts which in this application are optional, and add them to PACK_TEMPLATE programatically after the length field ... hmm, that's OK. No, create a function with the 2 optional parts as input and the string $pack_template as output.	[reply]
Re^4: Factoring out common code in two methods of a class by BrowserUk (Patriarch) on Aug 05, 2009 at 20:59 UTC
Re^3: Factoring out common code in two methods of a class by Anonymous Monk on Aug 06, 2009 at 00:47 UTC
I had wondered about "use constant..." but had it in my head that I would need a constant for each 'field' I often do things like this - that is, reject a solution prematurely based on perceived drawbacks. Usually I haven't thought about the problem in enough depth. I find it's very helpful to sit down and actually WRITE DOWN the requirements (or constraints) for a particular problem, then think through them in detail. (For me, the physical act of writing, be it with keyboard or pen, helps me engage in the thinking-through process.)	[reply]
Re: Factoring out common code in two methods of a class by metaperl (Curate) on Aug 05, 2009 at 17:18 UTC
building that format string might be clearer if moved into a simple templating engine. Why dont you look into using Moose to ease your OOP efforts? Also, methods dont "normally" return anything as much as they populate a slot in the object.	[reply]
Re: Factoring out common code in two methods of a class by BrowserUk (Patriarch) on Aug 06, 2009 at 02:29 UTC
further reading of the docs tells me that pack/unpack is not the way to do the bit-level twiddling. So I am looking into vec for the flag handling. Look at vec by all means, but don't be surprised if you come back to unpack, or switch to masking and shifting. The problem with vec is that it will only deal with powers-of-two numbers of bits. For example, there is no way to get at your 7-bit field directly using vec. Of course, unpack has its problems too. Getting a 7-bit field back as 7 individual 0\|1s isn't ideal. And you can get unintuative results if you attempt to unpack bits across byte-boundaries also. Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error. "Science is about questioning the status quo. Questioning authority". In the absence of evidence, opinion is indistinguishable from prejudice. RIP PCW	[reply]
Re^2: Factoring out common code in two methods of a class by jvector (Friar) on Aug 06, 2009 at 10:07 UTC
The problem with vec is that it will only deal with powers-of-two numbers of bits. For example, there is no way to get at your 7-bit field directly using vec. Aiee! I had found the existence of vec and had it in a tab on my browser but had not studied it at the time I wrote that update... I had also done a CPAN search ( for "bits", as I recall) and there was one bit-manipulating utils module there (I don't recall the exact name) that had the same restriction (only operable on fields of 1,2,4,8... bits). Since these data structures tend to be dreamed up by hardware/comms engineers who are concerned with cramming the maximum amount of "useful" information into the smallest amount of bits, that restriction is not very helpful. Getting a 7-bit field back as 7 individual 0\|1s isn't ideal. But it may be the least ugly/painful way. There's obviously MTOW to do it but not all are equally attractive. The last signature was better than this one	[reply]