tags:

views:

286

answers:

9

I have the following subroutine in Perl to substitute "abc" for "xyz" in a string:

sub mySubst {
    my ($str) = @_;
    $str =~ s|abc|xyz|ig;
    return $str;    
}

It works, but seems way too verbose for Perl. How can I tighten it up?

A: 

what is the sub for? just do it like this

$str =~ s|abc|xyz|ig;
ghostdog74
What is any sub for? I don't want to repeat the same code in multiple places. Also, I want something that returns the substituted string, not changes the original.
JoelFan
@ghostdog: This is not functionally equivalent to the code in the question. Your code modifies the string variable in-place. JoelFan's code will not modify a variable passed to the sub.
toolic
A: 

You can omit the return keyword:

sub mySubst {
    my ($str) = @_;
    $str =~ s|abc|xyz|ig;
    $str;    
}

It is also possible to use the default variable ($_) :

sub mySubst {
    local $_ = shift;  # or my starting from Perl 5.10
    s|abc|xyz|ig;
    $_;    
}
philippe
Although then you must localise `$_`, otherwise you will clobber the caller's `$_`.
Dave Hinton
I don't like assigning to `$_`. There is no need for it, let perl do it for you or create your own variable.
Evan Carroll
yes, make that `my $_ = shift`
ysth
No you have to use local $_ = shift ... "my" will not work
JoelFan
@JoelFan As of 5.10.0, `$_` can be lexicalized: http://perldoc.perl.org/perl5100delta.html#Lexical-%24_
Greg Bacon
Yeah, I just saw that... I haven't been keeping up with 5.10!
JoelFan
+3  A: 

You could write:

sub mySubst { (map { s|abc|xyz|ig; $_ } "$_[0]" )[0] }

but unless this is an exercise in obfuscation, I would say go with what you have. Remember, you are not writing the program for the computer.

Sinan Ünür
to get a string copy just use `"$_[0]"` instead of `my ...`
ysth
@ysth Thanks, that is a good idea.
Sinan Ünür
wow, what's with the downvotes? this is the best answer, while still saying it's not worth doing.
ysth
Why do you need the quotes around $_[0]? Also, you could replace that with @_
JoelFan
@JoelFan You need `"$_[0]"` to make a copy of `$_[0]` instead of operating directly on elements of `@_` (which are aliases to the values passed). You cannot just do `map { } @_` if (1) you want the argument not to be changed but a fresh string with substitutions to be returned and (2) if you want to be able to say `my $x = mySubst($y)` without `$x` getting assigned `1` and (3) if you want the function to operate on string literals as well (e.g. `my $x = mySubst('abc');`) For more information, look at the previous version in the edit history and my comments on the various answers here.
Sinan Ünür
Wow, and the quotes are necessary to prevent the aliasing... that is subtle!!
JoelFan
A: 

To avoid changing the originals, you could write

sub mySubst { map { (my $s=$_) =~ s|abc|xyz|ig; $s } @_ }

or

sub mySubst { my @a = @_; map { s|abc|xyz|ig; $_ } @a }

or to borrow from Sinan's answer, except discarding the temporary array explicitly:

sub mySubst { map { s|abc|xyz|ig; $_ } my(undef) = @_ }

but the syntax is still a little noisy. Because it uses map, be sure to call it in list context:

my($result) = mySubst $str;  # NOT my $one = mySubst $str;

If you expect to mostly call mySubst with a single argument but want to handle cases of one or more arguments, then you could write

sub mySubst {
  s|abc|xyz|ig for my @a = @_;
  wantarray ? @a : $a[0];
}

but that starts up the stuttering again.


If you want to update the parameter itself, use the alias semantics of Perl's subs as documented in perlsub:

The array @_ is a local array, but its elements are aliases for the actual scalar parameters. In particular, if an element $_[0] is updated, the corresponding argument is updated (or an error occurs if it is not updatable).

So you could write

sub mySubst { $_[0] =~ s|abc|xyz|ig }

or even

sub mySubst { map { s|abc|xyz|ig; $_ } @_ }

Example usage:

$str = "fooabcbar";
mySubst $str;
print $str, "\n";

Output:

fooxyzbar
Greg Bacon
I am assuming the fact that the OP's `mySubst` leaves the original string unaltered means that is a design criterion.
Sinan Ünür
@Sinan Thanks and edited. JoelFan stated it explicitly in response to ghostdog74's answer: http://stackoverflow.com/questions/2267312/sub-to-do-substitution-in-perl/2267390#2267390
Greg Bacon
@gbacon the two versions at the top of your post assign `1` to `$x` in `my $x = mySubst($y);`
Sinan Ünür
@Sinan Hence the warning to call `mySubst` in list context! :-)
Greg Bacon
@gbacon: Yes, I had somehow missed that warning, so +1. However, I do think this is the kind of function which should return the string in both contexts.
Sinan Ünür
+8  A: 

What you have is fine.

  • You pull the arguments off the @_ variable, making a copy, using a list assignment. List assignment is an excellent way to do it (and basically the standard way.) Using shift would also work, but changes @_ (may or may not be what you want.) There's a discussion on PerlMonks about shift vs @_ that you might be interested in.
  • You use a named variable with your search and replace. I prefer named variables in this case, since dealing with Perl's magic variables takes care.
  • Automatically working on $_ would be possible, but not having $_ auto populated makes it trickier to get right. You'd need to do local $_ = shift; or local ($_) = @_; which doesn't add much.)
  • I like it when people use an explicit return. It's a warm fuzzy.
  • K&R Brackets. Good. :)
  • No prototype. Very good. :)

Go with it. I think you're on the right track.

Robert P
shift does not give you an alias. it makes a copy just as the list assignment does. you have aliases only when working directly with @_
ysth
Whups, my bad. Fixing.
Robert P
you also get aliases if you re-alias @_, such as: for (@_)
JoelFan
I agree with you on most of your points but I still like Sean's answer, which reduces the code, at least somewhat :)
JoelFan
+6  A: 

If you're looking for golf, and not production code.

use strict;   ## Not really required ;)
use warnings; ## Not really required ;)
sub f{local$_=pop,s/foo/bar/ig;$_}
print f 'foobarbaz';
Evan Carroll
Save 2 by replacing shift with pop :)
toolic
Why the down votes, this is clearly not production code, but when you're trying to lessen verboseness without reason, I'm left only to assume this is a golf-esque type example. Why not indulge the questioner? This is a part of perl culture anyway.
Evan Carroll
+1 to offset downvotes, since you've clearly stated your context.
Axeman
+1 If it compiles/runs it's fair game for the codegolf tag. People who don't know what golf is shouldn't vote on golfed answers.
gnibbler
+1  A: 

I think too verbose sounds like not enough obfuscation which I disagree. As for tightening it up I'd suggest something along the lines of:

sub replaceBeginningWithEnd {
    my $text = shift;
    return if not defined($text);
    $text =~ s/abc/xyz/ig;
    return $text;    
}
  • Make names more readable
  • Use conventions (ie. / as oppose to |)
  • Argument checking
Maxwell Troy Milton King
Perl programmers don't use camelCase much, we write our `if not` s like `unless`, and we don't typically use parens around builtins like defined.
Evan Carroll
I like PascalCase myself, but to each his own. TMTOWTDI!
Robert P
How could Perl programmers not like Camel case? ;)
JoelFan
+3  A: 

This is how I would render that subroutine more idiomatically:

sub mySubst {
    (my $str = shift) =~ s|abc|xyz|ig;
    return $str;
}
Sean
Yikes! This is not idiomatic - it's just hard to read and maintain!
DVK
@DVK... please explain
JoelFan
@DVK: It IS idiomatic. See http://www.perlmonks.org/?node_id=366431 for an explanation and a reference to the Perl Cookbook. Although I should have also said that JoelFan's original snippet looks perfectly fine to me; my answer just tightens it up a bit.
Sean
@Sean - if you're sending me to the link to show that this code is valid perl - I already know that. I was merely pointing out that idiomatic - at least in my view - is a closed range (e.g. at some point, the code jumps from being merely idiomatic Perl to Perl Golf territory).
DVK
... Closed range being "Writing C/shell/whatnot code in Perl" -> Writing idiomatic Perl by using Perl specific code to enhance power -> writing Perl Golf at which point the code becomes hard to red and unmaintainable. If you consider that rightmost area to still be included into "idiomatic Perl", we'll just have to agree to disagree.
DVK
As an example, your life now sucks if you want to pass >2 parameyers - you will have to either break your expression, or process @_ parameter list in disjoint points in the code. The latter is not exactly good coding.
DVK
@JoelFan - see my example above. If you wish to pass in other parameters (e.g. string to search for or to replace with), this code structure is no longer workable (at least if you wish to follow good practices and process all members of @_ in one place), whereas your original structure is just fine.
DVK
@JoelFan - also, as per Robert's answer, shift changes @_ which may be OK but might cause problems in general. Again, the question is "what is the purpose of reducing the code" - in your case, any further reducing increases readability and maintenance costs while not achieving any purpose than reducing character/line count statistics.
DVK
@DFK: To repeat, the link provides evidence that my code, and in particular the construction ($foo = something) =~ s/.../.../ is, in fact, idiomatic Perl. And your objection to the shift() function is rather puzzling. But fine, use $_[0] instead if you prefer.
Sean
This might be better: `sub mySubst { (my($str) = @_) =~ s|abc|xyz|ig; $str; }`
Brad Gilbert
@Brad - did you test that? It doesn't seem to work.
JoelFan
+1  A: 

No one gave the Perl idiom for this yet. This replicates the behavior of the original code by modifiying a copy:

  (my $new_str = $old_str) =~ s/abc/xyz/ig;

If I had to put that into a subroutine, which I think is kinda silly for such a simple operation, I guess that would be:

 sub foo { (my $s = $_[0]) =~ s/abc/xyz/ig; $s }

However, if I was doing something silly like this, I wouldn't make a named subroutine for it. I'd make a subroutine to make the subroutine for me so I don't need a new named sub for every possible sort of replacement:

 sub make_sub {
      my( $regex, $replacement ) = @_;

      # season to taste with error checking
      sub {
          (my $s = $_[0]) =~ s/$regex/$replacement/ig; $s
          };
      }

 my $replacer = make_sub( qr/abc/, 'xyz' );

 my $new = $replacer->( $string );
brian d foy