Re: Is "ref($class) || $class" a bad thing?

[dragonchild]

merlyn has railed against this construct for years. Do a Super Search for "cargo-cult programming" and you'll find hundreds of nodes on the topic.

Basically, that construct is saying "This method can be called as either a class or instance method". While this may be neat (for example, Test::Cmd takes advantage of this by inheriting from File::Spec), it's certainly not helpful in most cases.

But, I must take exception with one of the conclusions you're implying. The error in line 14 is caused by the fact that your new() method isn't verifying that it was actually passed anything. The better solution is:

package Foo;

sub new {
    my $proto = shift || die "Must pass a class or object into new()";
    my $class = ref($proto) || $proto;
    bless {}, $class;
}

Of course, this still doesn't fully work because it doesn't take into account Foo::new( {} );, which will return an object blessed into the HASH class. So, maybe we should do something like:

package Foo;

use Scalar::Utils qw( blessed );

sub new {
    die "Must pass a class or object into new()" unless @_;
    my $proto = shift;
    my $class = blessed($proto) or $proto;

    bless {}, $class;
}

That should take care of most situations I can think of. But, we're just getting stupid. :-)

[hardburn]

Basically, that construct is saying "This method can be called as either a class or instance method".

merlyn's writings have been against using this in constructors specifically, not against using it in general methods to allow it to be called against both a class and an object. This is because a chunk of programmers out there expect it to act as a clone, another chunk thinks it should act as a copy, and yet another chunk thinks it should be an error.

I go for something like this:

sub new 
{
    my $class = shift;
    die "Need to be called as a class method" 
        if ref $class;
    my %params = %{ +shift };

    # Define what params we expect
    my %self = map { $_ => $params{$_} || '' } qw/
        foo bar baz
    /;

    # Validation

    bless \%self => $class;
}

[merlyn]

merlyn has railed against this construct for years. Do a Super Search for "cargo-cult programming" and you'll find hundreds of nodes on the topic.

I doubt that it is actually "hundreds". {grin}

Basically, that construct is saying "This method can be called as either a class or instance method".

But that isn't why I'm rallying against it. In my own perlboot docs, I show many examples of methods that can be called on either class names or instance values. Please see my recent column on constructing objects, especially the last few paragraphs, for the best description of specifically what I rally against.

[stvn]

But, I must take exception with one of the conclusions you're implying. The error in line 14 is caused by the fact that your new() method isn't verifying that it was actually passed anything.

I know thats why its happening, you know why its happening, but does Joe-Perl-Newbie know why? Maybe, maybe not. The error message given points back to the $foo->test line, which it not the actual source of the issue. IMO, its just adds to the insidiousness of this.

As for a way to solve this, code-wise, I am currently toying with using this:

sub new {
  my ($class) = @_;
  $class = ref($class) || $class;
  (UNIVERSAL::isa($class, __PACKAGE__) 
      || UNIVERSAL::isa(__PACKAGE__, $class)) 
          || die "Oh no you don't!!!!";  
  bless {}, $class;
}

It works (or rather doesn't work) in all the following conditions:

Foo::new();
Foo::new("SomeOtherModuleName") ;
Foo::new([]);
Foo::new({});

As well as works correctly when inherited. I am trying to think of other ways it might get used/abused to see how well it holds up. Any thoughts are appreciated.

But, we're just getting stupid. :-)

I know, isn't it fun ;-P

UPDATE:
I think this would actually be the most thorough approach, even though I think UNIVERSAL::isa probably handles $class being undef okay.

sub new {
  my ($class) = @_;
  $class = ref($class) || $class;
  (defined $class && 
      (UNIVERSAL::isa($class, __PACKAGE__) 
          || UNIVERSAL::isa(__PACKAGE__, $class))) 
              || die "Oh no you don't!!!!";   
  bless {}, $class;
}

[chromatic]

Seems like it'd be less work to write:

sub new
{
    croak "new() called as a function, not a method\n" unless @_;
    bless {}, shift;
}

If you really want to be paranoid, though, you should make something similar a precondition of all of your methods. (For convenience sake of everyone else, though, please use [CPAN://Scalar::Util]::blessed(). Also, you ought to call isa() directly, not through UNIVERSAL::, in case someone overrides it.

[stvn]

Seems like it'd be less work to write:

sub new
{
    croak "new() called as a function, not a method\n" unless @_;
    bless {}, shift;
}

As dragonchild already pointed out, this would not handle Foo::new({}), nor would it handle Foo::new("Data::Dumper"). Both are things are equally as stupid for a user to do, but a new user may not realize the method/function difference, the real question is whether that is my reponsibility or not.

If you really want to be paranoid, though, you should make something similar a precondition of all of your methods.

I assume you mean to check $self, that is true, but even I think that would a little much.

Also, you ought to call isa() directly, not through UNIVERSAL::, in case someone overrides it.

That too would not work for Foo::new({}) or other such insanity.

I am really just trying to find something in between totally over-the-top paranoid, and you-break-it-you-buy-it-an-I-dont-care. And I am still looking as I think the example I gave above it probably too much.

[adrianh]

I'm surprised nobody has mentioned called_as_method from Devel::Caller.

use Devel::Caller qw( called_as_method );

sub new {
    croak "must call new as method" unless called_as_method(0);
    ...
};