Beefy Boxes and Bandwidth Generously Provided by pair Networks
Clear questions and runnable code
get the best and fastest answer
 
PerlMonks  

inheritance turns back and bites

by dada (Chaplain)
on Mar 01, 2004 at 13:13 UTC ( #332883=perlquestion: print w/ replies, xml ) Need Help??
dada has asked for the wisdom of the Perl Monks concerning the following question:

can you tell me, looking at the following code:
  1. what is wrong
  2. the most elegant way to fix it
  3. what can be done to avoid similar errors
UPDATE: this is bad code, I know. it's just a completely fake example I made up for this post. it's also deliberately contrived, just to make the real problem less apparent (and where's the fun, otherwise? :-). you have been warned. don't do this at home.
package SomeData; # UPDATE: the first version of "new" doesn't seem to # work on some platform/version combination (and it's # way too evil anyway). but since this is not the main # issue, I will use a more "sane" version :-) # sub new { bless \pop, shift } sub new { my $self = pop; bless \$self, shift } sub value { ${shift;} } sub add { my($self, $type, $value) = @_; if($type eq 'number') { return $$self += $value; } if($type eq 'string') { return $$self .= $value; } die "only string or number allowed"; } sub increment { my $self = shift; $self->add(number => 1); } package MyNumber; use base qw( SomeData ); sub add { my($self, $value) = @_; die "not a number" unless $value =~ /^\d+$/; $$self += $value; } sub subtract { my($self, $value) = @_; die "not a number" unless $value =~ /^\d+$/; $$self -= $value; } package main; my $answer = MyNumber->new(42); print $answer->value(), "\n"; $answer->subtract(1); print $answer->value(), "\n"; $answer->increment(); print $answer->value(), "\n";
cheers,
Aldo

King of Laziness, Wizard of Impatience, Lord of Hubris

Comment on inheritance turns back and bites
Download Code
Re: inheritance turns back and bites
by borisz (Canon) on Mar 01, 2004 at 13:28 UTC
    you expect that increment call SomeData::add, but it calls MyNumber::add. Because it is a MyNumber object. If you do not want that, write in package SomeData
    sub increment { my $self = shift; $self->SomeData::add(number => 1); }
    Boris
      Always use SUPER instead of the parent-class-name; so in the case of changeing the parent-class, you won't have to replace all the "SomeData::" stuff to the new parent-class-name.
      SUPER is pretty super.
        But SUPER is wrong here, I like to use SomeData or build something with __PACKAGE__.
        Boris
Re: inheritance turns back and bites
by broquaint (Abbot) on Mar 01, 2004 at 13:30 UTC
    1. The $answer object is using its own method instead of SomeData's method
    2. Use Class::MultiMethods, or less elegantly
      sub add { @_ > 2 && return $_[0]->SUPER::add(@_[1 .. $#_]); ... }
    3. Don't use parameter based dispatch in perl ;)
    HTH

    _________
    broquaint

Re: inheritance turns back and bites
by tinita (Parson) on Mar 01, 2004 at 13:41 UTC
    as others said already, in MyNumber, you have implemented the method add different from the one in SomeData. you have to decide which one you want.

    also, in the new method of SomeData, you have a little error. if you do
    sub new { bless \pop, shift }
    it's like doing
    sub new { bless \42, "MyNumber" }
    and you can't bless a constant into an object. so do my $arg = pop; bless \$arg, shift

      sub new { bless \pop, shift } it's like doing sub new { bless \42, "MyNumber" } and you can't bless a constant into an object. so do my $arg = pop; bless \$arg, shift
      I do not think this is wrong, since \pop is like \pop @_ and whatever I pop it is pushed and a pushed constant is a scalar.
      Boris
        sorry, but:
        perl -wle' sub new { bless \pop, shift } my $main = new ("ClassName", 42);' Modification of a read-only value attempted at -e line 2. This is perl, v5.6.1 built for i586-linux This is perl, v5.8.0 built for i686-linux

        am i overlooking something?

        Update: same result with
        This is perl, v5.8.2 built for i686-linux

      it's like doing sub new { bless \42, "MyNumber" }
      no, it's like doing:
      sub new { bless $_[-1], $_[0] }
      which I can do. arguably a good idea, but I can. and in fact, that part works, at least in my perl interpreter. (note: the code is deliberately "strange" just to hide a bit the real problem :-).

      cheers,
      Aldo

      King of Laziness, Wizard of Impatience, Lord of Hubris

Re: inheritance turns back and bites
by dragonchild (Archbishop) on Mar 01, 2004 at 14:40 UTC
    For the specific question, a better option might be to use overload and do dispatch based on the value you get. Something like:
    package SomeData; use overload '+' => 'add', '-' => 'subtract', fallback => 1; sub add { my ($x, $y) = @_[0..1]; ($x, $y) = ($y, $x) if $_[2]; $self->{is_number} ? $x + $y : $x . $y; }

    Now, personally, I would never overload + to act as string concatenation. Concatenation isn't commutative, which breaks the implicit contract most people have with addition. As well, how do you define multiplication for strings?

    ------
    We are the carpenters and bricklayers of the Information Age.

    Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

Re: inheritance turns back and bites
by Abigail-II (Bishop) on Mar 01, 2004 at 14:54 UTC
    Well, you can't modify constants. Your problem isn't any different than:
    bless \42, "whatever";
    or
    sub foo {$_ [0] += 10} foo 42;
    Furthermore, the use of bless \pop, shift; is questionable, as there's no order of evaluation defined for the comma in list context.

    Abigail

      That's what I thought at first glance, but after a moment realized that pop, shift on a 2+ element array is going to get the last and first elements regardless of order of evaluation.

      Nevertheless, it's a bad way to do it, since if the method call is given no parameters, the bless will default to package "main" (with a warning).

Re: inheritance turns back and bites
by John M. Dlugosz (Monsignor) on Mar 01, 2004 at 16:13 UTC
    Nobody seems to have addressed the actual question, about the underlying reason for this being a mistake and how to avoid it.

    Basically, you used the same method name in a derived class as is used in the base class, and that accidently overrides a function that just happens to have the same name but is used for a different purpose, takes different parameters, etc.

    The first thing that comes to mind is to make sure all members are documented or follow a naming convention for "internal use" items. But, you can still hit that problem with an internal-use function! All methods that are called using the virtual dispatch mechanism, even those that are private, must be documented so a writer of a derived class is aware of them and will not re-use those names.

    —John

Re: inheritance turns back and bites
by stvn (Monsignor) on Mar 01, 2004 at 19:35 UTC

    Disclaimer
    Sorry, but first i have to rant. This is not nessecarily 100% about your code, but about a particular Perl-OO idiom I see often on this site.

    Maybe, I'm a prude, but I just don't understand how anyone could think that this idiom:

    # sub new { bless \pop, shift } sub new { my $self = pop; bless \$self, shift }

    is good OO. It completely externalizes the internals of the object in the worst way by allowing the caller of "new" to essentially create them. It does no form of parameter checking at all, and is lazy in the worst way (creates more problems later on, instead of solves them).

    Your code breaks in silently if I do any of these things:

    MyNumber->new([ 0 .. 100 ]); MyNumber->new({ test => 1}); MyNumber->new(*STDOUT);
    Now of course, these are "unrealistic" examples because your classname and documentation would indicate doing such things were just plain stupid. But, in a larger program (especially with a number of developers) variables may get trampled on, or return values are changed, or any other such issue. If the eventual argument to your constructor is just such a vicitim, your object willingly accepts it without complaint. And when your code suddenly die's (possibly) far away from where the actual bug was created, you may waste hours tracking it down.

    Now onto your questions:

    1. what is wrong
    2. the most elegant way to fix it
    3. what can be done to avoid similar errors
    Question "a" has already been answered, its because "increment" is using "add" from MyNumber and not from SomeData.

    Question "b": My suggestions is to not try and combine string handling and number handling into the same base class. Meaning that SomeData doesn't make alot of sense to me. Especially when you later derive MyNumber from it, and then change the number and type of parameters accepted by "add". This creates a very confusing issue/problem/bug in that MyNumber::add no longer acts the same as SomeData::add and therefore changes the interface.

    In a more static language like Java or C# these two methods would be considered different since their method signatures are different. In those langauges your code would work correctly as the call to "add" in "increment" would dispatch correctly to the two argument "add". But alas, this is perl, so I would suggest changing your class design to be more approriate to perls ways and means.

    Also, the concept of "increment" is clearly not appropriate to a string. I realize that you specifically call $self->add(number => 1) in the code, but it would seem to me that SomeData is intended to be a string/number handling hybrid base class of some kind, and that implies to me that SomeData should only implement methods appropriate to the both of strings and numbers.

    Question "c": see answer to question "b".

    -stvn
inheritance turns back and bites (for real this time)
by dada (Chaplain) on Mar 02, 2004 at 15:53 UTC
    time to clarify things up :-)

    I would like to thank everyone that answered to this rather silly post, and took part in a sort of bad joke of mine.

    I had prepared a very different kind of posting, talking about inheritance, interfaces and code maintenance, but in the end I decided to try to be thought-provoking and so posed the question as a sort of quiz. and I'm sorry if this caused more harm than good :-).

    almost everybody spotted the error in redefining the interface (or the signature, if you prefer to think in terms of it) for a method in a derived class.

    the rule I wanted to point out, which in Perl (due to its typeless and prototypeless nature, that we all sometimes love) may easily go overlooked (I, for one, didn't had it so clear in mind before stepping on it):

    Don't change the interface of methods you overload from a base class (that is, you can do things differently internally, but both input and output should be consistent with the original method) or inheritance will turn back and bite you.

    and that's my exposition of point c. regarding point b (a way to fix this), many (here and in other forums) suggested checking the number, and possibly the nature, of parameters in MyNumber::add, as per broquaint's:

    @_ > 2 && return $_[0]->SUPER::add(@_[1..$#_]);
    this has the disadvantage that one can still write: $answer->add(string => 'xxx'), and this may very well be what the author of MyNumber wanted to avoid.

    one could check the value of the second parameter, and die unless it is number, but this seems rather kludgy.

    a slightly less inelegant way to fix this problem could be for example:

    sub add { # check if the Big Boss is calling us... if( (caller(1))[3] =~ /^SomeData::/) { # we feel guilty, so let's hide return shift->SUPER::add(@_); } # ...back to our scheduled job my($self, $value) = @_; die "not a number" unless $value =~ /^\d+$/; $$self += $value; }
    but still broquaint's hint on Class::Multimethods wins this thread's elegance award.

    as a final note, I will also unveil where really I encountered the problem (so that you can finally forget about my broken example :-). I was taking a look at Net::ICal for a project of mine, and I found that Net::ICal::Time, which is intended to be just a wrapper class for Date::ICal, defines a method:

    add($duration)
    but Date::ICal has an add method, which has this synopsis (as well as others):
    add(duration => $duration)
    and that's exactly our point a. the code in Net::ICal::Time::add ends with:
    return $self->SUPER::add(duration=>$duration);
    but as we saw, this is not enough to be safe.

    to further underline how sneaky this bug can be, I will also add this: the code in Date::ICal::new, which Net::ICal::Time delegates to, calls offset, which in turns calls add (which is really Net::ICal::Time::add) only when it has to do some adjustment to account for timezones.

    and that's all for today. I hope you found all this somewhat entertaining :-)

    cheers,
    Aldo

    King of Laziness, Wizard of Impatience, Lord of Hubris

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://332883]
Approved by giulienk
Front-paged by gmax
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others exploiting the Monastery: (7)
As of 2014-10-25 15:41 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    For retirement, I am banking on:










    Results (145 votes), past polls