Code Check
michael at diaspora.gen.nz
michael at diaspora.gen.nz
Tue Apr 29 03:14:40 CDT 2003
>I've just made a brief pass through it and it looks pretty
>good to me - especially for a 'novice'. Here are a few
>random thoughts that struck me as I read through. As they
>are all style related and style is a matter of personal
>taste, I won't be in the least offended if you wish to
>ignore or debate them.
Please, take the following in the same spirit. Particularly the good
for a "novice" bit; it's quite clear that you've programmed in other
languages before, and are just a Perl novice.
>Well, that's enough from me. I'll be interested to see what
>others thought.
I'll just focus on one bit, the loop that looks like:
READ: while (<ITEMFILE>) {
$eline = $_;
if ($eline =~ /span class="rss:item"/) {
$span_complete = $false;
foreach $eline (<ITEMFILE>) {
if ($eline =~ /\/span/) {
$span_complete = $true;
last READ;
}
$eline =~ s/\&/\&/g;
$eline =~ s/\/span//g;
$eline =~ s/<\/div>//g;
$eline =~ s/</\</g;
$eline =~ s/>/\>/g;
$eline =~ s/"/\"/g;
$eline =~ s/'/\'/g;
$itemline[$j] = $eline;
$j++;
}
}
}
I'd be inclined to write that something like this:
sub SPAN_NOT_SEEN { -1 }
sub IN_SPAN { 0 }
sub SPAN_COMPLETE { 1 }
...
$span_state = SPAN_NOT_SEEN;
for (<ITEMFILE>) {
if (/span class="rss:item"/ .. m!/span!) {
$span_state = IN_SPAN;
s!</div>!!g;
s!/span!!g;
s!'!'!!g;
push @itemline, CGI::escapeHTML($_);
if (m!/span!) {
$span_state = SPAN_COMPLETE;
last
}
}
}
... do something with $span_state
Several comments:
(1) I hate leaning toothpicks (the "s/\/span//g" in the original),
and always prefer changing the delimiters.
(2) Whenever I see "$array[$counter] = $item; $counter++", I
always replace that with push, as I can get rid of a variable
that way, and that's Good.
(3) The flip-flop operator is a cool piece of Perl syntactic sugar;
it's reasonably easy to understand what it does from looking at it,
but if you haven't used sed, you won't have seen it before.
(4) I'd check CGI::escapeHTML to see if it also does quoting of
', which it might well do -- then you could get rid of another
line.
(5) $false and $true strike me as very wrong; either use 1 and 0,
or at least something like:
sub FALSE { 0 }
sub TRUE { 1 }
and use case to distingush them as globals. You've actually got
three cases here, rather than the implied boolean of "$span_complete",
so I renamed it to "$span_state".
More information about the Wellington-pm
mailing list