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

[stvn]

There is a discussion going on over on the perl-qa list about Devel::Cover and if 100% coverage is a realistic goal or not. The most common reason for not getting 100% coverage is the common OO-perl idiom of

my $class = ref($class) || $class;

In order to write a test that would cover this, you would have to include the following in your test code:

my $foo = Foo->new();
my $foo2 = $foo->new();
my $foo3 = Foo::new();

This would then satisfiy Devel::Cover's condition coverage fully, and cover all 3 variations:

+------+-------+-------+
| true | false | final |
+------+-------+-------+
|   0  |   0   |   0   |  Foo::new()
+------+-------+-------+
|   0  |   1   |   1   |  Foo->new()
+------+-------+-------+
|   1  |   x   |   1   |  $foo->new()
+------+-------+-------+

But it occured to me that while this would give you sought after 100% coverage, it not only did nothing to improve the quality of the test, but it allowed a (potential) bug to slip through.

Consider this code:

package Foo;
sub new {
  my ($class) = @_;
  $class = ref($class) || $class;
  return bless {}, $class;
}

sub test { print "Test" }

package main;
my $foo = Foo::new();
print $foo;
$foo->test();

It's output (IMO) is not a good thing.

main=HASH(0x180035c)

The hash that was supposed to be blessed into the Foo package, has been blessed into main instead. No error, no warning, no indication that what has happened is incorrect. Only when an attempt is made to call a method though the $foo instance do we find out what is wrong, with the not-very-helpful error message of:

Can't locate object method "test" via package "main" at test.pl line 1
+4.

Line 14 is $foo->test(), thats not very helpful. Sure I know what happened, and most reasonably experienced perl hackers will see the problem pretty quickly (especially if they are familiar with OO-perl). But that still leaves out a portion of the population which are just left scratching their heads.

Back in perl-qa, someone posted a link to this paper How to Misuse Code Coverage, which i recommend reading if you are currently using code coverage in any of your projects.

The author of the paper basically points out that code coverage tools can give a false sense of security to your tests, that bad test design can still produce good coverage. That sometimes there are errors/issues/incidents which are not actually handled in the code, and therefore cannot be tested, and hence your code coverage tool cannot tell you that the code that ought to exist is covered.

This then brings me back to my $class = ref($class) || $class; and achieving 100% coverage with it. In reality, many modules out there call other methods in their constructor, and hence will die when you call Foo::new(). This can be tested easily like this:

eval { Foo::new() };
ok(!$@, "Foo::new dies when called as a function");

You can get your 100% and sleep comfortably at night knowing that all is right in the world.

But what about all the modules out there which don't call other methods in their constructors? Maybe, it time to re-think this common idiom? What say you O fellow monks?

[Ovid]

The ref $proto || $proto idiom is not bad in and of itself. It's bad when it's tossed in around blindly without documentation because it suggests that perhaps the programmer is unaware of the intention. If, however, the programmer has a reason for this idiom and documents this reason (even if only through tests), then I'm less concerned. For example, one person mentioned that they frequently use factory classes to pass objects to other modules than then call new on instances of those objects to spawn further objects. This can be a reasonable approach and if it's documented and tested, no problem.

As with anything in Perl (or even life), I would hate to automatically declare anything wholly without merit (with the possible exception of bell peppers -- they're just gross), so I'd argue that this idiom has it's place so long as someone knows why they've put it there.

Of course, in the interest of full disclosure, I'm the guy who submitted the patch that had the ref $proto || $proto code mostly removed from the latest perldocs.

[stvn]

The ref $proto || $proto idiom is not bad in and of itself. It's bad when it's tossed in around blindly without documentation because it suggests that perhaps the programmer is unaware of the intention.

But that implies that people will read said documenation and even more so that if they read it, they would understand it. That can be a heavy assumption. Of course, it could be argued that if they don't read it, it's their problem anyway.

Personally, I tend to be a paranoid programmer, and I am very concerned about my modules working as I intend and sending feeback/warnings/errors back to the user if they are not used correctly, regardless of the documenation.

As with anything in Perl (or even life), I would hate to automatically declare anything wholly without merit (with the possible exception of bell peppers -- they're just gross), so I'd argue that this idiom has it's place so long as someone knows why they've put it there.

I guess I am not arguing so much that the idiom is bad, but that maybe teaching it soooooo widely is. I know that I use it, and have never really stopped to consider the implications of it being misused (I mean, all the cooool kids use it ;-) . So yes, I am one of those people who didn't stop to think about why I put it there, but I am sure I am not alone. And I am also sure there are many new perl programmers, who will make the same mistake. When will the madness end!!!!!!

Of course, in the interest of full disclosure, I'm the guy who submitted the patch that had the ref $proto || $proto code mostly removed from the latest perldocs.

Excellent! Did the patch get accepted?

[Ovid]

I don't expect programmers to automatically know they can call $object->new and if they try it without reading the docs then I would be in the "serves 'em right" camp. Does that do the same thing as calling new() on a classname (like all other class methods do?) or does it make a clone of the object? If it clones it, is it a deep or shallow copy? Do the two instances really share the same filehandle? There are just two many questions to be answered for something like that to not be documented.

The patch for removing references to this from the perldocs was accepted with almost no disagreement. I was very surprised by that. Given how contentious the debate is, I was certain that more people on P5P would speak out against the patch.

[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.

[chromatic]

This function call passes nothing:

my $foo = Foo::new();

What do you expect to be in $class in this code?

sub new {
  my ($class) = @_;
  $class = ref($class) || $class;
  return bless {}, $class;
}

What do you expect to be in $class in this code?

sub new
{
    my $class = shift;
    bless {}, $class;
}

I don't see the connection.

[kscaldef]

The point, such as it is, is that in order to cover all three possible evaluations of the conditional, you have to make that meaningless call. This, of course, sucks. Now, probably the answer is that 100% code coverage isn't always possible or desirable.

On the other hand, I've seen large (hundreds of classes) systems where every constructor included ref $class || $class, and nowhere was any of them ever called using an existing instance. So, that's just silly.

If you never use the constructor in this way, you might as well just drop it. As a minor side-effect, you get your code coverage a little higher, and maybe that makes your manager happy or something.

[stvn]

I am not sure I understand your point. I think you are trying to say that

$class = ref($class) || $class;

is no better/worse than

my $class = shift;
bless {}, $class;

which is quite true, and since that is also a common idiom which will produce the same issue, then maybe that is not so good either.

In all cases, $class is undef, and when bless is passed undef it blesses into main. It might (and I stress might, because I am not so sure of this) make sense if when bless encountered undef in its second argument, that it treated it as a one-argument bless instead (which would result in blessing into the current package).

[chromatic]

I'm suggesting (as I think your conclusion supports) that if you have to abuse your API to test the possibilities that your code should support, you have a mismatch somewhere.

There are reasons to call methods as subroutines in tests, but those are rare. If you've not violated Liskov substitutability, you can probably subclass appropriately. (If you have violated LSP, you should rethink your design. :)

[eric256]

Would $class = ref($class) || $class || __PACKAGE__; make it work in that case?

[hossman]

my $class = ref($class) || $class;

In order to write a test that would cover this, you would have to include the following in your test code:

my $foo = Foo->new();
my $foo2 = $foo->new();
my $foo3 = Foo::new();


Right off the bat, that's a missleading statement.

Code coverage tools can only tell you what lines are executed or not executed, I've never seen one smart enough that you can run it on a test suite, and have it tell you if all the side effects of every executed line of code is also executed.

If I was running Devel::Cover on some unit tests I wrote, and it told me there was a conditional in which one branch wasn't getting run, I would never assume that just writting a test that executed that branch is enough to consider that branch "tested". I would also look at any state modified in that branch, and make sure that my test included any other code that refrenced that state after the branch.

That's not just an important thing to remember about Code Coverage -- it's important to remember about any unit tests. I'm constently seeing people design/modify classes or objects which contain state, and write little micro-unit-tests which test each method individually, without ever testing "chains" of method invocations. The worst example I ever saw was an implimentatin of a glorified Hash that had methods like "add($key,$val)</code, <code>get($key), remove($key) and listKeys()" This is what the unit tests looked like...

function testAdd() {
    $hash = new Hash;
    assert($OK == $hash->add("foo","bar"));
}
function testGet() {
    $hash = new Hash;
    $hash->add("foo","bar");
    assert("bar" == $hash->get("foo"));
}
function testList() {
    $hash = new Hash;
    $hash->add("foo","bar");
    assert("foo" == $hash->listKeys());
}
function testRemove() {
    $hash = new Hash;
    $hash->add("foo","bar");
    assert($OK == $hash->remove("foo"));
}

The unit tests all passed, the code was handed off to the client, and the client send back the following set of tests *they* had written to prove that the damn library was a load of crap...

function testGetShouldNotDieHorriblyIfKeyNotFound() {
    $hash = new Hash;
    $hash->get("foo");   # this was causing a core dump
}
function testRemoveShouldAcctuallyRemove() {
    $hash = new Hash;
    $hash->add("foo","bar");
    $hash->remove("foo");
    assert("bar" != $hash->get("foo"));  # hey look, still there
}

Just becuase you're code is getting executed, doesn't mean it works, or that all of the permutations of use cases are getting executed.

[stvn]

Code coverage tools can only tell you what lines are executed or not executed, I've never seen one smart enough that you can run it on a test suite, and have it tell you if all the side effects of every executed line of code is also executed.

I agree with you 100%, as I said in the OP

But it occured to me that while this would give you sought after 100% coverage, it not only did nothing to improve the quality of the test, but it allowed a (potential) bug to slip through.

The paper I linked to drives how this point even more, if you haven't already, you should give it a read, I thought it was quite good.

[gmpassos]

There's only sence to your class bless for other classes (when you get $class as an object), if this other class can inherit over your class. So, if you get an object blessed into the main package, like your example, your package shouldn't create a object for it.

I think that to keep OO ok you should declare new() as:

sub new {
  my $class = ( @_ ?
                    ( ref($_[0]) ?
                                  ( UNIVERSAL::isa($_[0] , __PACKAGE__
+) ? ref(shift(@_)) : __PACKAGE__ )
                                  :
                                  ( $_[0] =~ /^\w+(?:::\w+)*$/ ? shift
+(@_) : __PACKAGE__ )
                    )
                    :
                    __PACKAGE__
              ) ;
  
  my $this = bless {}, $class ;
  
  ## initialyze code...
  
  return $this ;
}

I know that is not a beautiful code, but to do what you want is that.