Stories
Slash Boxes
Comments
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • by Ovid (2709) on 2002.09.11 16:41 (#12763) Homepage Journal

    Off the cuff, here's how I would transform this stuff and why.

    if ($dbh) {
      return 1;
    } else {
      return 0;
    }

    That would become:

    return 1 if $dbh;

    Unless there is a demonstrable need to return zero for false, I consider it a bug. What if the results are assigned to an array? The array then has one element, zero, which causes an if (@array) conditional to incorrectly evaluate as true. Further, since boolean tests don't warn about undefined values, why bother to ever return the zero? Unless that number is a count, it's probably not necessary -- and it's a bug waiting to happen.

    $searchsql = sprintf("%s%s",$searchsql,$sql);

    Wow. What a waste:

    $searchsql .= $sql;

    It's much easier to read and some maintenance programmer isn't going to come along and try to figure out if there is something deeper going on. I was sitting here trying to see if there was something deeper going on, but all this is doing is writing obfuscated code.

    while ($stopsearch == 0) {
    my $rv = $sth->execute || die $dbh->errstr;
    # loads of commented out code
    if ($rv eq '0E0') {
      $stopsearch = 1;
    } else {
      # lots more code
      $stopsearch = 1;
    }
    }

    This becomes something along the lines of:

    unless( $stopsearch ) {
      my $rv = $sth->execute; # RaiseError should probably have been set in the constructor
      if ( '0E0' eq $rv ) {
        some_error_routine( $rv, $dbh );
        last;
      }
      # do my stuff
    }

    That's just roughly thrown together and may not match what you need, but it's easier to read. However, just the way $stopsearch is appearing before the 'execute' makes me a wee bit curious. I wonder if there is a deeper issue here. Also, what are the search parameters? Are they static? If not, I saw that there were no arguments passed to the execute method, so I am wondering if $dbh->quote was called on the arguments earlier? Just a thought ...

    Idiomatic Perl code is easier to read and it's less likely to be buggy. This means money saved in both development and maintenance. If you start writing unit tests, you can possibly show how these things can fail by creating the proper test cases. Anyone who writes code this sloppy is probably creating plenty of bugs. Then, if you can pull that off, rewrite the code correctly and rerun your tests. Thus, you can gain the benefit of testing and teaching idiomatic Perl.

    Fair warning: this is politically risky. If you're not very well regarded (and it sounds like that may be a problem if they're not listening to you), then this could get you branded a trouble maker.

    Or you could take a route I took a couple of years ago. The vice president of my company was constantly moving crud into production and I would silently rewrite it so that it worked. He never noticed :)