1. First the bug. You are not getting and setting the element you think you are. Your RE needs to be fixed to do that.
2. The improvement. Your get/set methods are going to be slow. Usually an AUTOLOAD is best off creating the method and then calling that so that future invocations will run faster. This is worth some increase in the complexity of your method. (Sorry jeffa, they had not cached their methods properly for that speedup.)
3. From here on it, it is all about style. You aren't using strict. Inside the AUTOLOAD that is often quite reasonable. But you should at least unimport it there so that it will work fine when dropped in a module that does use strict.
4. As I have mentioned elsewhere, I am not a fan of making constructors callable by prototype. I think it is healthy to distinguish class methods from instance methods. Even when it wouldn't be overly confusing to allow that, I don't see a point in allowing it for the simple reason that the kind of functionality you are likely to want when constructing a new instance from an old is likely to be different some day.
5. jeffa's point of having an empty DESTROY method is good.
6. I see no reason to create your AUTOLOAD routine by assigning it to a typeglob. The usual declaration is enough.
7. When you are doing as much as you are (and more when I get through with it), I see no reason to try to play golf. Break stuff out a little so people can see what is going on.
8. I don't find myself using get/set methods very much. If you are putting them in classes mechanically, it is worth asking why. While I don't entirely agree with this article, he does have a lot of good things to say. (I disagree with his dismissal of the value of having different views of the same object. I have done that. I think it is a very valuable thing to do. If most programmers have not done that, that is only evidence of the fact that the value of so doing is not better known. But that is a topic for another day.)

package PKG;
use strict;
require Carp;

sub new {bless {}, shift;}

sub DESTROY {}

sub AUTOLOAD {
  no strict;
  if ($AUTOLOAD=~/(\w+)$/) {
    my $field = $1;
    *{$field} = sub {
      my $self = shift;
      @_ ? $self->{$field} = shift
        : $self->{$field};
    };
    &$field(@_);
  }
  else {
    Carp::confess("Cannot figure out field name from '$AUTOLOAD'");
  }
}
[download]

I agree about not wanting to make a potentially tricky routine like this into Perl golf... though the title was short version of AUTOLOAD and it certainly lived up to that...

One minor thing; wouldn't you be better off doing a


 goto &$field;
[download]

so that the call stack doesn't show the AUTOLOAD? After all, if you've created the subroutine, the AUTOLOAD isn't significant any more.

It is traditional to put a goto there for exactly that reason. However this time I know it won't matter.

Besides which a friend of mine demonstrated to me that jumping from sub to sub with goto could lead to a memory leak (he wrote a tail-recursive sub with a goto and after several runs with large argument lists ran out of memory) so I don't use that goto unless I really feel it is necessary for Perl to work correctly.

I don't know if that bug has been reported to p5p yet. At the time I intended to check whether it still existed in a development version and then report it, but then I got busy...

The first is the ability to cache an AUTOLOAD'ed function. By assigning an anonymous subroutine to the typeglob *{$AUTOLOAD}, you actually create the subroutine 'on the fly' in the package's symbol table - this means that only the first call to this method will suffer the 'find the right sub' performance hit. But I jumped the gun, as tilly pointed out, your method indeed does not add the method name to the symbol table. (thanks for providing the snippet tilly)

The second is being able to 'overload' the method so you don't have to specify if you wish to 'get' or 'set' an attribute. I tried to do the same thing, and failed miserably. :) Now i see how to do it (you just create one sub that determines which of two actions to take).

The whole line starts out with a ternary operator. The question is - do the contents of $AUTOLOAD match the regex, and if so, is the number of arguments passed to this 'method' greater than 1?

The match catches everything except DESTROY, and as a side effect stores the attemped method name in $1. For example $object->foo() would be looked up as PKG::foo, and $1 would contain 'foo'.

This match takes place on the left hand side of the &&, and as a result, executes to see what the result is: if true, $1 indeed contains the matched 'method' and the right hand side of the && is evaluated.

The size of the argument list is checked via $#_, which returns the index of the last element in the list. Knowing that the first argument is always the name of the object, it is clear that if $#_ is greater than zero then the client is issuing a 'set' - if not, the client is issuing a 'get'.

The true part of the ternary operator describes the behavior for the 'set' function:

for example:                 $_[0]{$1}=$_[1]      
would roughly translate to:  $self->{foo} = 'bar'
if the method call was:      $object->foo('bar');
[download]

for example:                 $_[0]{$1}
would roughly translate to:  return $self->{foo}
if the method call was:      $object->foo()
[download]

sub DESTROY {}
[download]

(thanks to bikeNomad for class/object clear-up)

Jeff

R-R-R--R-R-R--R-R-R--R-R-R--R-R-R--
L-L--L-L--L-L--L-L--L-L--L-L--L-L--