search and replace

Y

yeti349

Hi. The following snippet seems to be doing what I want, but I'd like
to get suggestions for refinement or improving the code. Thank you.

#!/usr/bin/perl -w

use strict;

my $phrase = "MARKEM 1ST HOLDINGS INC. & MONEY MARKET LLC";

my @terms = terms();

foreach my $term (@terms)
{
$phrase =~ s/\W/ /ig; #remove nonword characters
$phrase =~ s/\S*\d\S*/ /ig; #remove numeric strings
$phrase =~ s/\b($term)\b//ig; #remove business terms
}

#new phrase: "MARKEM MONEY MARKET"
print "$phrase\n";

sub terms
{
my @terms = qw(holdings inc llc);
}
 
P

Paul Lalli

Hi. The following snippet seems to be doing what I want, but I'd like
to get suggestions for refinement or improving the code. Thank you.

#!/usr/bin/perl -w

You enabled Warnings. Very good. However, these days, it is
preferable to write
use warnings;
rather than the -w option on the shebang.
use strict;
Excellent!

my $phrase = "MARKEM 1ST HOLDINGS INC. & MONEY MARKET LLC";

My personal preference is to not use double quotes unless you actually
need interpolation.
my @terms = terms();

I get nervous when using the same identifier for multiple pieces of
information. There's nothing *wrong* with this, of course, and Perl
will readily do exactly what you want. It just seems needlessly
confusing to the reader. Consider something like `my @terms =
get_terms();` instead.
foreach my $term (@terms)
{
$phrase =~ s/\W/ /ig; #remove nonword characters

Your comment doesn't match what the code actually does. You're not
removing non-word characters. You're replacing each non-word character
with a single space character.

There is no reason for the /i modifier in that regexp.

You're specifying the /g modifer, which means this one single statement
removes *all* the non-word characters. So why is this line in a loop?
This code does not in any way depend on $term, and it should be moved
outside the loop so that it's only executed once.
$phrase =~ s/\S*\d\S*/ /ig; #remove numeric strings

Pretty much the same critiques for this one. The comment doesn't match
what's happening, the /i modifier is pointless, and it should only be
executed once.
$phrase =~ s/\b($term)\b//ig; #remove business terms

Okay. *This* one should absolutely remain in the loop, and should use
the /i modifier, and it is in fact actually removing the term.

My only critique here is that there's no reason to use (), as you're
not using $1 anywhere.
}

#new phrase: "MARKEM MONEY MARKET"
print "$phrase\n";

Again, your comment doesn't match. That is not the new phrase. The
new phrase is: 'MARKEM MONEY MARKET '
sub terms
{
my @terms = qw(holdings inc llc);
}

If this subroutine has no point other than returning a list of values,
why are you bothering to create a new variable at all?


Putting all my critiques together, we're left with:
#!/usr/bin/perl
use strict;
use warnings;

my $phrase = 'MARKEM 1ST HOLDINGS INC. & MONEY MARKET LLC';

my @terms = get_terms();

$phrase =~ s/\W/ /ig; #replace non-word chars with a space
$phrase =~ s/\S*\d\S*/ /ig; #replace strings containing a digit
#with a space

for my $term (@terms) {
$phrase =~ s/\b$term\b//ig; #remove business terms
}

#new phrase: 'MARKEM MONEY MARKET '
print "$phrase\n";

sub get_terms {
qw/holdings inc llc/;
}

__END__
 
A

Anno Siegel

[most of excellent code revision snipped]
I get nervous when using the same identifier for multiple pieces of
information. There's nothing *wrong* with this, of course, and Perl
will readily do exactly what you want. It just seems needlessly
confusing to the reader. Consider something like `my @terms =
get_terms();` instead.

I don't get nervous when the like-named things represent essentially
the same information. As this is the case here, I might have used
"terms" in both senses without compunctions.

However, in this case the issue can be dodged. The variable @terms is only
used once, so it can go (okay, there are exceptions). Just call terms()
directly in its place.

foreach my $term ( terms() ) {

Anno
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top