FAQ
Hi Anirban,

a few comments on your code.
On Thursday 02 Jun 2011 08:06:54 Anirban Adhikary wrote:
Hi list

I have wirtten the following perl code which has some configuration
params such as prefix,suffix,midfix and nofix. The logic is filenames
will be selected based on prefix,suffix and midfix pattern.After the
fiter through above 3 patterns if file names exists with nofix
patterns it will be removed from the final list.

use strict;
use warnings;
It's good you're using strict and warnings.
#my $pfx ="CAT";
my $pfx = '';
my $sfx = '.Z' ;
my $mfx = 'r';
my $nfx= "dmp.Z,Q.Z,dat.Z,ctl.Z,A.Z";
my @fnames =
("CAT_arcdf231456_dmp.Z","ABCD_1234.txt","FILE_STAT.Z","CAT_qwerty.Z");

my($re_pfx,$re_sfx,$re_mfx,$re_nfx);
my(@pre_fix,@sfx_fix,@mid_fix,@not_fix);

if((length($pfx)) > 0 and $pfx !~ /^\s+$/) {
@pre_fix = split(/,/,$pfx);
#find_largest_element(@pre_fix);
$re_pfx = join "|",@pre_fix;
$re_pfx = qr/$re_pfx/;
}
if((length($sfx)) > 0 and $sfx !~ /^\s+$/) {
@sfx_fix = split(/,/,$sfx);
$re_sfx = join "|",@sfx_fix;
$re_sfx = qr/$re_sfx/;
}

if((length($mfx)) > 0 and $mfx !~ /^\s+$/) {
@mid_fix = split(/,/,$mfx);
$re_mfx = join "|",@mid_fix;
$re_mfx = qr/$re_mfx/;
}
1. You have quite a lot of duplicate code here. You can extract it into a
function call, a hash and/or a loop.

2. You may wish to use http://perldoc.perl.org/functions/quotemeta.html .

3. You should have a space between the "if" and the "(".

4. You can just say «if (length($mfx) and $mfx !~ /\A\s+\z/)».
if((length($nfx)) > 0 and $nfx !~ /^\s+$/) {
@not_fix = split(/,/,$nfx);
$re_nfx = join "|", @not_fix;
$re_nfx = qr/$re_nfx/;
}
my @listed;
You should have an empty line between "}" and the "my @listed";
foreach my $fname(@fnames) {
if($fname =~ /^.*?$re_nfx$/) {
#print "FILE NAME [$fname] is not a valid file \n";
next;
}elsif ($fname =~ /^$re_pfx/){
push(@listed,$fname);
}elsif ($fname =~ /^.*$re_sfx$/) {
push(@listed,$fname);
}elsif ($fname =~ /^.*$re_mfx.*$/) {
push(@listed,$fname);
}elsif() {
push(@listed,$fname);
}else {
push(@listed,$fname);
}
1. Your indentation here is bad.

2. You can combine the elsif into several clauses using "or" or "||".

3. It's better not to cuddle the else's and elsif's:

} elsif ($fname =~ /^$re_pfx/) {
push(@listed,$fname);
} elsif ($fname =~ /^.*$re_sfx$/) {


Regards,

Shlomi Fish

--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
First stop for Perl beginners - http://perl-begin.org/

I hope that you agree with me that 99.9218485921% of the users wouldn't bother
themselves with recompilation (or any other manual step for that matter) to
make their games run 1.27127529900685765% faster ;-) -- Nadav Har'El

Please reply to list if it's a mailing list post - http://shlom.in/reply .

Search Discussions

Discussion Posts

Previous

Follow ups

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 2 of 3 | next ›
Discussion Overview
groupbeginners @
categoriesperl
postedJun 2, '11 at 5:07a
activeJun 2, '11 at 8:56a
posts3
users3
websiteperl.org

People

Translate

site design / logo © 2021 Grokbase