David Eason wrote:
>
> [snip]
>
> Any feedback is welcome.
>
> [snip]
>
> splits2d.zip, which contains splits2d.pl and supporting files (including a
> sample input file bookmarks.html to play with), can be found at this link if
> anyone's interested:
> URL:http://www.webneed.net/~heydave/work-in-progress/
The code looks pretty good over all, just a couple of minor nits. :-)
1) You should test your code with warnings and strict enabled.
$ perl -wc splits2d.pl
"my" variable $header_text masks earlier declaration in same scope at splits2d.pl line
127.
122 sub read_html_files()
123 {
124 my $link_per_target_count;
125 my $header_text = $opts{suggest_category};
^^^^^^^^^^^^^^^
126 my $p;
127 my ($header_text, $dd_text, $got_dd_text, $href_val, $a_text, $tok, $tag);
^^^^^^^^^^^^^^^^
2) You should declare lexical variables where you define them or in the smallest
possible scope.
18 my ($tick, $tock, $time_elapsed);
[snip]
269 # start elapsed time measurement
270 $tick = [gettimeofday];
271
272 parse_args();
273 read_html_files();
274 write_html_files();
275
276 # end elapsed time measurement
277 $tock = [gettimeofday];
278 $time_elapsed = tv_interval $tick, $tock;
These three variables are declared 251 lines away from where they are actually used.
218 sub write_html_files() {
219
220 my ($category, $i, $link, $page);
These variables are declared at the top of the sub instead of where they are used so
you haven't noticed that $i is no longer used in this sub. Also, it is more efficient
to declare and define them at the same time.
122 sub read_html_files()
123 {
124 my $link_per_target_count;
156 ++$link_per_target_count;
It doesn't look like you need this variable.
129 my $doc;
130 foreach $doc (@ARGV)
Would be better as: foreach my $doc (@ARGV)
32 my $app_name = basename $0;
$app_name is not used anywhere.
3) You are using sprintf when concatenation would do.
230 $write_file = sprintf ("$target_base%d$target_ext", $target_count);
Could be written as:
$write_file = "$target_base$target_count$target_ext";
236 linkToPrev(sprintf ("$target_base%d$target_ext", $target_count - 1));
Could be written as:
linkToPrev( $target_base . $target_count - 1 . $target_ext );
240 linkToNext(sprintf ("$target_base%d$target_ext", $target_count + 1));
Could be written as:
linkToNext( $target_base . $target_count + 1 . $target_ext );
HTH
John
--
use Perl;
program
fulfillment
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
