On Tue, Mar 25, 2008 at 1:47 PM, Bobby <[EMAIL PROTECTED]> wrote:
> Hi,
>
> I need another set of eyes to help look over the codes below. I have a
> subroutine that check for a comma in $strA and do some regex replacement, if
> there's no comma then return the values of the two arguments passed to it.
> The IF in my sub worked but the Else part didn't return any thing when i have
> records in my data set that doesn't have a comma in it. Records #(pid) 1,4,5
> didn't returned anything (else statement); the rest did the text substitution
> correctly(If statement). Thanks in advance for any suggestions.
snip
> if ($PSC_L0 =~ m/[^a-zA-Z0-9]/){
> my $PSC_Level = 'PSC_L0';
> my $strURL0 = &replaceComma($PSC_L0,$PSC_Level);
> print OUT "$strURL0";
snip
That is because you are only calling replaceComma() if $PSC_L0 has a
character other than a-z, A-Z, or 0-9 in it. Also, you are doing
several bad/odd things:
snip
sub replaceComma{
snip
Mixed case is bad (this is a style issue, so feel free to ignore this
advice, but it will annoy many seasoned Perl coders).
snip
my ($strA)= shift;
my ($strLevel) = shift;
snip
Why are you putting shift in a list context? The shift function
doesn't do anything special in list context, so just say
my $str = shift;
my $level = shift;
or
my ($str, $level) = @_;
snip
my $strPSC;
snip
This variable is not used in this context, it doesn't belong here. It
is also has negative value in the contexts where it is actually used,
so just don't create it in the first place.
snip
if($strA =~ m/,/){
snip
Don't use m unless you are specifying the delimiters for the match
(like m{}), it is just extra typing and more stuff to read.
snip
$strPSC = ",(AND($strLevel:";
$strA =~ s/,/$strPSC/g;
$strA =~ s/,/)),/g;
snip
This is a waste of typing and execution time, you could just use the
string directly in the substitution like this.
$strA =~ s/,/,(AND($strLevel:/g;
Also, I am willing to bet the second substitution doesn't do what you
want it to unless you want the output to be
a)),(AND(PSC_L0:b)),(AND(PSC_L0:c)),(AND(PSC_L0:d
I think you will find you can replace both substitutions with this one
and actually get the string you want:
$strA =~ s/,([^,]+)/,(AND($strLevel:$1))/g;
which produces
a,(AND(PSC_L0:b)),(AND(PSC_L0:c)),(AND(PSC_L0:d))
snip
$strPSC = "$strLevel:$strA)";
return $strPSC;
}
else {
$strPSC = "$strLevel:$strA)";
return $strPSC;
}
snip
In addition to the bad indenting (which I hope is an artifact of copy
and pasting the code into the email), why are you assigning to a
variable just to return the value? This is a waste of typing and
execution time. In both cases you can just return the string:
return "$strLevel:$strA)";
In fact, since the return values are identical, you should consider
refactoring your code to move the return out of the if/else (removing
the need for the else completely).
snip
}#end sub replace
snip
If you are going to do this sort of comment at least make sure you
spell the name of the function correctly (replaceComma).
snip
#Call sub replaceComma
#$PSC_L0 is read in from a text file, see sample above
if ($PSC_L0 =~ m/[^a-zA-Z0-9]/){
snip
This if is the culprit in the bug you are trying to squash. Only the
records with commas make it past this if statement.
snip
my $PSC_Level = 'PSC_L0';
snip
This is wasteful, just use the string. Unless you are doing something
with it or it appears in many places there is no benefit to putting it
in a variable (in fact, you have to type more and you waste execution
time and memory).
snip
my $strURL0 = &replaceComma($PSC_L0,$PSC_Level);
snip
Do not call functions with & in front unless you know exactly why you
are doing so.
snip
print OUT "$strURL0";
snip
Why are you putting $strURL0 in quotes? Do you realize that the code
that actually gets executed now looks like this:
print OUT join('', $strURL0);
You are making a worthless function call. Also, don't use the old
style bareword file handles. Use the new (well, newer, they have been
around for the last seven years) lexical file handles:
open my $out, ">", "outputfile.txt"
or die "could not open outputfile.txt: $!";
.
.
.
print $out $strURL0;
Here is how I would write your code (assuming
replace_comma_if_exists() is used in many places):
#!/usr/bin/perl
use strict;
use warnings;
sub replace_comma_if_exists {
my ($s, $level) = @_;
$s =~ s/,([^,]+)/,(AND($level:$1))/g;
return "$level:$s";
}
my $output_file = "out.txt";
open my $out, ">", $output_file
or die "could not open $output_file: $!";
while (<DATA>) {
chomp;
my ($pid, $psc_l0) = split /\|/;
print $out replace_comma_if_exists($psc_l0, "PSC_L0"), "\n";
}
__DATA__
1|A
2|A,F,G,H
3|A1,a2
4|F
5|G
6|A,G
If replace_comma_if_exists is only used here then it would be better
to just use the substitution directly and refactor it into a
subroutine later if needed (save on the overhead of a function call).
#!/usr/bin/perl
use strict;
use warnings;
my $output_file = "out.txt";
open my $out, ">", $output_file
or die "could not open $output_file: $!";
while (<DATA>) {
chomp;
my ($pid, $psc_l0) = split /\|/;
$psc_l0 =~ s/,([^,]+)/,(AND(PSC_L0:$1))/g;
print $out "$psc_l0\n";
}
__DATA__
1|A
2|A,F,G,H
3|A1,a2
4|F
5|G
6|A,G
--
Chas. Owens
wonkden.net
The most important skill a programmer can have is the ability to read.
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
http://learn.perl.org/
