in reply to How many bugs can *you* find

OK, in five minutes, trying not to remember what anyone else said: 
sub updateTiles { 
        my $fto = $htmDir . 'tile.htm'; 
## global var used

        my $content = ''; 
        open(HOME,$fto); 
## no checking for return value, could have redirect or pipe opens

        while (<HOME>) { $content .= $_ } 
## inefficient

        close(HOME); 
        my $paramTemp,$contentTemp; 
## $contentTemp is NOT BEING DECLARED LOCAL (very misleading)

        my @sections = qw(Tile Pile Link); 
        foreach $section (@sections) { 
## no declaration of $section

                $contentTemp = $query->param($section); 
## use of global $query.  Why is contentTemp not declared here?

                if ($section eq 'Pile') { $contentTemp =~ s/[\n\r]/<p>
+/g; } 
                if ($section eq 'Link') { $contentTemp = "<img src=\"i
+mages/enter.gif\" width=8 height=12><a href=\"cgi-bin/show.cgi?action
+=showTiles&tileType=Search&searchFor=$contentTemp\">View this month's
+ tiles.</a>" } 
## ampersands not entitized,  inserted content not entitized or escape
+d

                $content =~ s/<!--$section-->(.*)/<!--$section-->$cont
+entTemp/; 
## parens not needed on .*, what if $section has regex chars?

        } 
        open(HOME,">$fto"); 
## no checking return values; what if $fto starts with >?

        print HOME $content; 
## could get IO error.  What if visitor hits page while partially writ
+ten?

        close(HOME); 
## could get IO error.

        my $image = $query->param('Image'); 
        if ($image ne '') { my $newFile = fileUpload('Image',250000,1,
+'latest_image','JPEG','.jpg','.jpeg') } 
## image might be undef if param not provided.

}

[download]
This code is clearly not -w or strict compatible.

See what you can get for $10 of my time? How many of those would you have found for $10 of your time? {grin}

-- Randal L. Schwartz, Perl hacker

Replies are listed 'Best First'.
(Ovid) Re(2): How many bugs can *you* find
by Ovid (Cardinal) on May 01, 2001 at 03:25 UTC

    New business idea: post code samples and try to get merlyn to offer free code reviews. Sell code reviews to clients...

    merlyn wrote:

    See what you can get for $10 of my time? How many of those would you have found for $10 of your time? {grin}

    Rather than list what I found, I'll list what I didn't. Then everyone can see what a faker I am :)

       my $paramTemp,$contentTemp; 
## $contentTemp is NOT BEING DECLARED LOCAL (very misleading)

    [download]

    Gah! I didn't see that one. (FYI: if you don't see it, "my" binds tighter than the comma).

       $content =~ s/<!--$section-->(.*)/<!--$section-->$contentTemp/; 
## parens not needed on .*, what if $section has regex chars?

    [download]

    Saw the useless parens (and that despicable dot star!), but didn't think about $section having regex characters.

        if ($image ne '') { my $newFile = fileUpload('Image',250000,1,'lat
+est_image','JPEG','.jpg','
+.jpeg') } 
## image might be undef if param not provided.

    [download]

    Sigh. I missed this one, too.

    All in all, I don't feel bad about catching most of the errors. This little post was 17 lines of code. Imagine expanding this out to over 2,000!

    Cheers,
    Ovid

    Update: It's interesting to notice that someone can hack together a script that has virtually every line of code in error and have the script still work. The person who wrote this code was a coder, not a programmer.

    Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

[reply]
[d/l]
[select]
      Said merlyn, and then Ovid: 
         $content =~ s/<!--$section-->(.*)/<!--$section-->$contentTemp/; 
## parens not needed on .*, what if $section has regex chars?

      [download]
      Saw the useless parens (and that despicable dot star!), but didn't think about $section having regex characters.
      Actually, $section can't have regex characters as the code is written, since it simply iterates over the hard-coded items in @sections.

      That will be five dollars, please. I take paypal, and I don't take American Express. 

         MeowChow                                   
               s aamecha.s a..a\u$&owag.print
[reply]
[d/l]
[select]
        Actually, $section can't have regex characters as the code is written, since it simply iterates over the hard-coded items in @sections
        As written, but not as maintained. {grin}

        For this to pass code review for me, there'd either need to be a fix to that line to make it work even when $section has regex chars, or a note put up above saying "don't ever include regex chars in this string... see below".

        The reason I flag that is that it creates an intertwining dependency that will be hell for the maintenance programmer down the road. Trust me: I've been that maintenance programmer enough times to know how much I hate those things. That's why my code reviews are so thorough. {grin}

        -- Randal L. Schwartz, Perl hacker

[reply]
Re: Re: How many bugs can *you* find
by buckaduck (Chaplain) on May 01, 2001 at 19:49 UTC
    Wow. By my calculations, that's $120 an hour. And yet, I'm tempted to scrape up some money, gather a bunch of my scripts together, and see what Randal can do with them in an hour...

    buckaduck

[reply]

In Section Meditations