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:
http://mail.perl.org.il/pipermail/perl/2007-December/009327.html 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 ) instead. 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++) { #$count+=1; 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 . 8. <<<<<<< 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 element. 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": http://oreilly.com/catalog/9780596001735/ Take it with a grain of salt, but make sure you think about what Conway says there. Regards, 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 read. _______________________________________________ Perl mailing list [email protected] http://mail.perl.org.il/mailman/listinfo/perl
