RE: [code-review] Bencode.pm

> -----Original Message-----
> From: Sam Kington [mailto:sam-pj/Jc9goILWIgYZ2jlX3PFpr/1R2p/[email protected]]
> Sent: Wednesday, October 01, 2003 6:54 PM
> To: Code Review
> Subject: Re: [code-review] Bencode.pm
> 
> Caleb Epstein wrote:
> 
> > I'm soliciting comments on the following Perl module,
> > Bencode.pm, which implements in Perl "bencode"
> > serialization and deserialization routines.
> 
> What, no POD?

I concur; you should document in the manual-page style of all CPAN modules.
Liberal documentation is important for any production code. It should be considered *part* of the code.

In fact, I frequently start new modules by writing the docs first, as a way of templating.
Then I come back and tweak as the design evolves.

> Also, what's that #! -w stuff doing at the top of the module?
> AFAIK shebang switches only apply to scripts, not subsequently loaded
> modules.

If it isn't the first 2 characters of an executable that's being loaded by the shell, then it's just a comment to Perl. It doesn't hurt anything really; it might have an effect on subsequent runs of Perl on that module, but that's really only useful for syntax checking, and is only likely in a strange envorinment like a batch file, so it's not likely at all, but in those rare possible cases, might actually have been intended. :)

But I doubt it, since a path was included.


> Also, I personally prefer to only export symbols if they're explicitly
> specified. Even when I use modules that export stuff by default, I
> tend to fully-specify function names in production code (as opposed 
> to quick and dirty hacks), because it makes it easier to work out where 
> that function was defined.

As a rule of thumb, never export ANYTHING by default. Make it @EXPORT_OK and maybe add 
  %EXPORT_TAGS = ( all => [ @EXPORT_OK ] );
but don't arbitrarily stomp around in the user's namespace.

> As for @+: 
> 
> > substr ($x, $f) =~ /^(0|-?[1-9][0-9]*)e/;
> > return int ($1), $f + $+[0];
> 
> Why say $+[0] rather than length($1)+1? (I haven't 
> benchmarked the two.)
> length($1)+1 is more obvious, and more backwards-compatible, 
> to my mind.

Go for the readability unless it just won't do the job.

> I also dislike the preponderance of one-character variable names.
> We're no longer using 1980s BASIC where only the first character of 
> the variable mattered, and RAM is cheap. We can afford to give more 
> descriptive names to variables.

Likewise, consider the poor schmucks in the future who may have to debug or maintain or edit this code -- if they want to implement a change in a variable's use, they might want to use a search utility to find all occurences of @R. With just a small change, @R is almost surely references as $R[$x] somewhere.... Now searching for it means seperately looking for @R and $R, or just looking for R, which is likely to show up in the POD you need to add....

On the other hand, @temp_list is easier to search for without false hits, easier to read....
And you might even be able to come up with a better name than @temp_list.

> Inconsistency: why fatal errors in decode_int, decode_string 
> and decode_dict but not in decode_list?

Be consistent, and when you need to be inconsistent, put the reasons in the documentation, and in comments around the site of variation.

> Checking whether something is an integer:
> > $x =~ /^(0|-?[1-9]\d*)$/
> 
> Is that quicker and/or more reliable than int($x) eq $x? 
> (Again, I haven't benchmarked.)
> Can you get integers reported in scientific notation?

If you want to avoid warning messages from invalid comparisons caused by -w elsewhere in the code, try { local $^W = undef; die $msg unless int($x) eq $x; }
 
> For that matter, the decoding functions get passed a buffer 
> and an offset. It's not clear how large this buffer is going to be, but 
> given that this is a BitTorrent client I imagine buffers are going to be fairly 
> large. Do you want to hold all of that in memory? Is it more efficient to 
> call substr on a scalar, or to read from a filehandle? (Which could well be a socket, a
> blessed scalar or something else fancy.)

You might also want to consider passing in a reference, which causes less memory shuffling.
Benchmark aliasing if you want to avoid the constant dereferencing.
I've seen code that does stuff like this:

  sub foo { 
      local (*val) = @_;
      print $val;
  }

But I haven't benchmarked it.

*****
"The information transmitted is intended only for the person or entity to which it is addressed and may contain confidential, proprietary, and/or privileged material.  Any review, retransmission, dissemination or other use of, or taking of any action in reliance upon, this information by persons or entities other than the intended recipient is prohibited.  If you received this in error, please contact the sender and delete the material from all computers."  113