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 .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
