Both of your alternatives (one-line-ish, and using a sub) are very hard to read, and require lots of duplicate code for each message type. Hide the gory details, leaving a clean interface (such as those below). This process is called encapsulation.

A procedural interface:

my $TEST_REC_FORMAT = get_msg_format [
   status => 'n  ',   #   0
   time   => 'n  ',   #   2
   date   => 'N  ',   #   4
   code   => 'a16',   #   8   key
   msid   => 'a10',   #  24   key
];

my %test_vals = parse_msg($TEST_REC_FORMAT, $rec);
[download]

An OO interface:

my $TEST_REC_FORMAT = MessageFormat->new([
   status => 'n  ',   #   0
   time   => 'n  ',   #   2
   date   => 'N  ',   #   4
   code   => 'a16',   #   8   key
   msid   => 'a10',   #  24   key
]);

my %test_vals = $TEST_REC_FORMAT->parse($rec);
[download]

What exactly get_msg_format or new returns is not important. How exactly it is calculate is not important either.

It's so very hard to read. What's important is to hide the gory details and provide a neat interface

And that's why I like Parse::FixedLength. The interface is very similar to what you have. I used to rewrite the same gory details over and over (similar to what the OP has) until they were stuffed into that module.

I take it the initial reaction then is that someone reading the code won't mind going elsewhere to understand what's going on, or won't need to do so because the function names are clear enough. (Or if they do mind a little, this is out-weighed by greater ease in writing the code.)

In general you should aim to localise the understanding of the code to the current context. By that I mean, the bit of code you are looking at now should be written so its purpose is clear without having to refer to other materials.

Generally that means rewriting something like:

setOptions (1, 0, 'e');
[download]

as:

setOptions (send_log => 1, suppress_warnings => 0, max_level => 'e');
[download]

A little more verbose, but now (even without the context) it's pretty clear what the parameters do and what setOption is likely to achieve.

Or to paraphrase a great man: make your code as clear as possible, but no clearer :).

---

DWIM is Perl's answer to Gödel

That's my opinion. Even if the function is simple, I still wouldn't copy it over and over, because I believe it's a distraction from the rest of the code. If you're going to go this way, stuff the function in in a module called OurCompany::FixedLengthUtils or something, and then no one has to think about how it works, they can just use it, or look up the docs for it.

And, I would probably create a separate module/class for each format especially if you'll be reusing the same formats in different scripts (which Parse-FixedLength has instructions on doing -- disclaimer: I'm the author :-)

I find unclear the way the two lists are interleaved. In a style similar to that hinted at in GrandFather's comment, I'd tend to write this as list of hashrefs:

my @TEST_REC_PACK_FIELDS = (
  { name => 'status', data => 'n' },
  { name => 'time', data => 'n' },
  { name => 'date', data => 'N' },
  { name => 'code', data => 'a16' },
  { name => 'msid', data => 'a10' },
);
[download]

At this point you can just as easily construct the list of fieldnames and the unpack string, but it is more self-documenting, and you can easily extend the structures later if you need to start adding validation callbacks and the like.

Hope this helps,

Hugo

It isn't a large amount of work to duplicate the functionality, but the time it takes to document your new module and the time it takes to put together a comprehensive test suite can be substantial.

Someone else has already done this work, so it would be better to use their existing code and test suite. They might have thought of rare cases that you would have missed. And if you know of a case they don't test for, I'm sure the maintainer would appreciate a well written test case for it.

I myself would do it in an OO way (let's call the class for format templates 'Unpack::Format'):

my $format = Unpack::Format->new( spec =>[
   status => 'n',   #   0
   time   => 'n',   #   2
   date   => 'N',   #   4
   code   => 'a16',   #   8   key
   msid   => 'a10',   #  24   key
]
);

#overloaded
@test_vals{ @$format } = unpack "$format", $rec;

# pure OO style
@test-vals{$format->fieldnames) = unpack $format->templatestr, $rec;
[download]

UPD: according to Hofmator's note, name-format pairs must be passed as an array reference. It seems to be a very easy typo to make :)

UPDATE 2: Working implementation (tested)

In the way it is written, your solution suffers from the same problems as BrowserUK's solution: The order of the fields gets lost by using a hashref as the spec-argument to Unpack::Format::new.

-- Hofmator

Update: As pointed out below, this doesn't work! Sorry, A rush of blood I think.

If you used a hash to contain your spec:

my %TEST_REC_PACK_FLDS = {
   status => 'n  ',   #   0
   time   => 'n  ',   #   2
   date   => 'N  ',   #   4
   code   => 'a16',   #   8   key
   msid   => 'a10',   #  24   key
};
[download]

Then your field names becomes just

use constant @TEST_FLDS => keys %TEST_REC_PACK_FLDS;
[download]

and you build your format string using

use constant TEST_REC_PACK => $join ' ', values %TEST_REC_PACK_FLDS;
[download]

You might want to uppercase the field names.

If you need your keys to be in a certain order, then I wouldn't use a hash in this way (unless it's one of those tied hash modules). An array is the appropriate thing here.

Field order is lost by using a hash.

Thanks, I meant to mention that the order of the fields is important for the list of field names. (I've added an update mentioning that.) I considered using tied hashes that preserve order, but thought that would be less clear.

If I just needed to preserve the order of the unpack spec, I could also use the "@0 n @2 n @4 N @8 a16 ..." form of unpack() specs, but the fieldname order is also useful to us.