[Israel.pm] A Few Notes Regarding rpmdig version 1.3

Shlomi Fish shlomif at iglu.org.il
Tue Apr 14 14:31:49 PDT 2009

Hi Mr. Andrejkovic!

I ran into your rpmdig program on Freshmeat.net ( 
http://freshmeat.net/projects/rpmdig/ ) the other day and mentioned it on the 
Israeli Perl Mongers mailing list (CCed to this message) here:


Here are some comments on your Perl code:

1. Your script begins with:

#!/usr/bin/perl -w
#use strict;

You shouldn't have commented out "use strict". Instead fix the problems that 
you encounter with the Perl code.

2. You include the entire text of the GPLv3 license inside single-quotes. 
Using single quotes for such long, multi-line text is unreliable. You should 
use here-documents (see "<<EOF" in http://perldoc.perl.org/perlop.html ) 

Otherwise, I should note you shouldn't include the entire licence in your 
script, because it unnecessarily bloats it. You can put the same notice as the 
one in the comment, and include the licence in a separate COPYING file.

3. This loop is badly written:

for (my $f=0; $f<=$#ARGV; $f++) {
  if ("$ARGV[$f]" =~ /^((-q)|(--quiet))$/ ) {$vF=0; next;}
  if ("$ARGV[$f]" =~ /^((-v)|(--verbose))$/ ) {$vF=1; next;}
  if ("$ARGV[$f]" =~ /^((-vv)|(--veryverbose))$/ ) {$vF=2; next;}
  if ("$ARGV[$f]" =~ /^((-vvv)|(--debug))$/ ) {$vF=3; next;}
  if ("$AR

Consider using Getopt::Long , and you may wish to assign $ARGV[$f] to 
something in the future and match against that. You should also use \A and \z 
instead of ^ and $, and use (?:...) for clustering instead of capturing 
parentheses (which are unnecessary there). Also consider using a dispatch 
table in the future.

4. You have a lot of if ( COND) { CLAUSE } in the same line. That's bad form. 
Please put the clause in a separate line, or use a trailing if modifier, if 
it's short.

5. You may wish to look into perltidy and tidyview (see 
http://sourceforge.net/projects/tidyview ) to reformat your code.

6. {{ $rpmname_=$rpmname; }} - what is the difference between $rpmname_ 
(underscore) to $rpmname? Name it something more meaningful.

7. You should name your variables better than %dbqfH .


sub upush { ($stack, $value) = @_;
 #Unique PUSH
 if (!grep {$_ eq $value} @{$stack}) { push (@{$stack}, $value); }

This is better done using an object that points to an arrayref and a hashref.

Also see http://xrl.us/beoq4z for more about searching an array for an 

9. You shouldn't use one-space indents. 4 seems to be the standard, and is 
easier to read.


In short, your Perl code is incredibly sub-optimal. Uneducated people who look 
at it, may inhibit many wrong ideas and bad practices from it. Please correct 
it in time for the next release of rpmdig.

For further discussion on what consitutes good Perl code, I refer you to 
Damian Conway's book "Perl Best Practices":


Take it with a grain of salt, but make sure you think about what Conway says 


	Shlomi Fish

Shlomi Fish       http://www.shlomifish.org/
What Makes Software Apps High Quality -  http://xrl.us/bkeuk

God gave us two eyes and ten fingers so we will type five times as much as we

More information about the Perl mailing list