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/\&/\&amp;/g;
		$eline =~ s/\/span//g;
		$eline =~ s/<\/div>//g;
		$eline =~ s/</\&lt;/g;
		$eline =~ s/>/\&gt;/g;
		$eline =~ s/"/\&quot;/g;
		$eline =~ s/'/\&apos;/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!'!&apos;!!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
    &apos;, 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