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

Levenglick Dov-RM07994 dov at freescale.com
Wed Apr 15 07:30:39 PDT 2009


Hi Mr. Fish,
Do you really think that an admonishment on a public mailing list is the
proper way to deliver such important information?

 
Best Regards,
Dov Levenglick
SmartDSP OS Development Leader
-----Original Message-----
From: perl-bounces at perl.org.il [mailto:perl-bounces at perl.org.il] On
Behalf Of Shlomi Fish
Sent: Wednesday, April 15, 2009 00:32
To: Jan Andrejkovic
Cc: Perl in Israel
Subject: [Israel.pm] A Few Notes Regarding rpmdig version 1.3

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
Perl at perl.org.il
http://mail.perl.org.il/mailman/listinfo/perl


More information about the Perl mailing list