Beefy Boxes and Bandwidth Generously Provided by pair Networks
Think about Loose Coupling
 
PerlMonks  

comment on

( [id://3333]=superdoc: print w/replies, xml ) Need Help??

Looks pretty good to me already! That said, here's a few suggestion for what I'd do differently, personally:

  • You can assign to a variable and apply a substitution in one step. Also, I personally find character classes a bit more readable when pruning several different characters from a string, so I'd write the assignment to $line like this:

    # Get rid of all line endings, regardless of which flavor they are (my $line = shift) =~ s/[\r\n]//g;
  • There's no need to declare variables until you use them - this will save some typing later on.

  • The checks for S1/S2/S3 records are all very similar; you could combine them into one (and then apply an extra sanity check to make sure that you have the right address length for your record).

  • Speaking of checks, I'd also just return from the sub if a check doesn't pass - less indentation later on. :)

  • What's more, there is a Unicode property for hex digits that you can use - \p{IsXDigit}, which is equivalent to [0-9a-fA-F]:

    # does this look like a S1, S2, or S3 record? These are the only o +nes that # carry an actual data payload. Other record types do not, so just + ignore # them. return unless $line =~ m/^s([1-3])(\p{IsXDigit}{2})(\p{IsXDigit}{4 +,6,8})(\p{IsXDigit}+)(\p{IsXDigit}{2})$/; my ($type, $count, $address, $data, $cs) = ($1, $2, $3, $4, $5); # does the address length match the record type? return unless length $address == 2 * ($type + 1); # Make sure the COUNT field makes sense. It should represent the s +um of # address, data, and checksum fields, in bytes return unless hex($count) == (length($address) + length($data) + l +ength($cs)) / 2;
  • Your $compCs calculation loops are pretty C-ish. You could write them more idiomatically by using m//g to split the string (in list context, it returns all matches); this would also allow you to combine them into one:

    my $compCs = hex($count); foreach my $nibble ($address =~ m/../g, $data =~ m/../g) { $compCs += hex($nibble); }

    Of course you could also use a statement modifier:

    my $compCs = hex($count); $compCs += hex($_) foreach($address =~ m/../g, $data =~ m/../g);

    If you do this, you should localize $_ in your subroutine so that it doesn't get clobbered in the calling routine.

    Personally, I find using map and reduce (from List::Util) the most readable:

    my $compCs = reduce { $a + $b } map { hex $_ } ($count, $address =~ m/../g, $data =~ m/../g);
  • You can set $status to the result of the comparison. In fact, this will also remove the need to explicitely set it to zero above; if the comparison fails, it'll be set to the empty string (a false value):

    my $status = $compCs == hex($cs);

So that's how I'd do it. Just for the sake of convenience, here's my version of this subroutine, in full:

use List::Util; # ... # Parse a single line of S-record, validate the line, and extract the +address # and data fields. Return a list of three scalars: # 1. TRUE if extraction was successful, FALSE otherwise (i.e. not a v +alid data # carrying S-record, bad s-record length, bad s-record checksum). # 2. The address of the line's payload, as a string. # 3. The data of the line's payload, as a string. sub ProcessSrecLine { # Get rid of all line endings, regardless of which flavor they are (my $line = shift) =~ s/[\r\n]//g; # does this look like a S1, S2, or S3 record? These are the only o +nes that # carry an actual data payload. Other record types do not, so just + ignore # them. return unless $line =~ m/^s([1-3])(\p{IsXDigit}{2})(\p{IsXDigit}{4 +,6,8})(\p{IsXDigit}+)(\p{IsXDigit}{2})$/; my ($type, $count, $address, $data, $cs) = ($1, $2, $3, $4, $5); # does the address length match the record type? return unless length $address == 2 * ($type + 1); # Make sure the COUNT field makes sense. It should represent the s +um of # address, data, and checksum fields, in bytes return unless hex($count) == (length($address) + length($data) + l +ength($cs)) / 2; # Make sure the checksum makes sense. It is the two's complement o +f # the sum of count, address, and data fields my $compCs = reduce { $a + $b } map { hex $_ } ($count, $address =~ m/../g, $data =~ m/../g); $compCs %= 256; $compCs = 255 - $compCs; # if the checksum actually matches, then call it a good S-record my $status = $compCs == hex($cs); return ($status, $address, $data); }

All of this is just my personal preferences and style, of course. As always, TIMTOWTDI (there is more than one way to do it), and I'm hardly an experienced Perl programmer anyway, so any advice I give may well fly in the face of established best practices. Caveat lector. :)

Another note - all this is completely untested, too.

EDIT: of course, there's also no need for a $status variable if you only assign to it once and then return, too:

# ... # if the checksum actually matches, then call it a good S-record return ($compCs == hex($cs), $address, $data);

In reply to Re^3: Parsing Motorola S-Rec file by AppleFritter
in thread Parsing Motorola S-Rec file by gri6507

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":



  • Are you posting in the right place? Check out Where do I post X? to know for sure.
  • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
    <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
  • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
  • Want more info? How to link or How to display code and escape characters are good places to start.
Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others about the Monastery: (4)
As of 2024-04-25 20:55 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found