Beefy Boxes and Bandwidth Generously Provided by pair Networks
XP is just a number
 
PerlMonks  

Re: Things I Don't Use in Perl

by demerphq (Chancellor)
on Aug 24, 2005 at 08:10 UTC ( #486123=note: print w/ replies, xml ) Need Help??


in reply to Things I Don't Use in Perl

Answer quickly! Does the following set the name or not?

$object->name(@name);

Well, its not clear to me that this behaviour is a bad thing at all. As written its conceptually similar to

$object->name(@name) if @name;

You could keep the dual purpose behaviour but make it more sensitive to this type of error quite simply:

use Carp; sub name { my $self = shift; if (@_) { $self->{name} = shift; return $self; } elsif (!defined wantarray) { croak "Error:", (caller(0))[3], " called in void context with no arguments"; } else { return $self->{name}; } }

Your point about using id() as a setter when it isnt meant to be used that way doesnt do much for me either. If the id() sub isnt meant to set the value then it shouldn't and it should throw an error when its asked to. Which puts the dual-use solution in the same position as set_id() would be. (IE it would be a run time error, one caught by the code, one caught by perl).

Also the two method approach has a problem that the dual use approach doesnt that you havent mentioned at all: in the two method approach you have two code stubs whose responsibility it is to operate on the same data. This means that if you change the field name or the objects construction you have to change two subroutines. With the dual use approach all of the logic to manipulate a given atttribute of an object is encapsulated in one sub. That means that things like validation rules are right next to the fetch logic, which means that when you are reviewing code you can see all of the "rules" that pertain to a given attribute in one place. You don't have to remember to review both. You don't have to independently review the setters validation logic in order to determine what the fetcher will return.

Personally i find the encapsulation offered by the dual-use approach combined with careful construction to be better practice than separate-subs.

Updated: to add the missing defined, as noted by radiantmatrix.

---
$world=~s/war/peace/g


Comment on Re: Things I Don't Use in Perl
Select or Download Code
Re^2: Things I Don't Use in Perl
by radiantmatrix (Parson) on Aug 24, 2005 at 16:28 UTC

    As implemented, your example would fail if the called in scalar context, as in $name = $object->name;, because wantarray will return false if you've called in scalar context. To check for void context, you need to check if the result of wantarray is undefined. This illustrates how easy it is to make a mistake with single get/set methods. Also, what if it is valid to set the name property to an empty string, the string '0' or undef? How would you do that with this combined get/set? Oh, you could play with wantarray to find out if you're getting, but what if (as is the case in the original, and common) the set method is to return the value set? Wantarray will be defined, so you have no way of reliably knowing if you were really called as get or set.

    It is more readable, safer, and simpler to code:

    sub set_name { my $self = shift; ## validation /untainting here ## $self->{name} = shift; return $self->{name}; } sub name { my $self = shift; if (@_) { warn "Attempt to set value in get" } return $self->{name}; }
    <-radiant.matrix->
    Larry Wall is Yoda: there is no try{} (ok, except in Perl6; way to ruin a joke, Larry! ;P)
    The Code that can be seen is not the true Code
    "In any sufficiently large group of people, most are idiots" - Kaa's Law

      As implemented, your example would fail if the called in scalar context, as in $name = $object->name;, because wantarray will return false if you've called in scalar context.

      Good catch.

      To check for void context, you need to check if the result of wantarray is undefined. This illustrates how easy it is to make a mistake with single get/set methods.

      Personally I feel this is a better illustration of how it is unwise to post untested code when you haven't even finished your morning cofee. Of course I'm biased. :-)

      Also, what if it is valid to set the name property to an empty string, the string '0' or undef? How would you do that with this combined get/set?

      With the bug you noticed squished the code presented does all of that doesn't it? If $obj->name(0) is called then it $obj->{name} gets set to 0, likewise with undef. The case of interest is when its called with an empty list.

      Oh, you could play with wantarray to find out if you're getting, but what if (as is the case in the original, and common) the set method is to return the value set?

      Here it is again slightly modified to match your code:

      So if we call it with arguments it either returns the old value, or returns self, whichever flavour you like. In fact your code (which is below)

      has an interesting property given the example before:

      my $val = $obj->set_name(@foo);

      If @foo contains something then $val and $obj->{name} ends up equal with $foo[0]. But if @foo is empty $val and $obj->{name} go to undef. Thus $obj->set_name() is equivelent to $obj->set_name(undef), but with the combined setter, $obj->name() returns the original value of the field, unless its in void context where it is an error. To me its quite arguable which of these two behaviours is to be preferred, and i might even go so far as to suggest that the posted implementation of set_name() should have something like

      die "No arguments in set_name()" unless @_;

      and possibly use it as a way of illustrating how easy it is to make a mistake in a split get/setter. :-)

      ---
      $world=~s/war/peace/g

        Interesting points. To be fair, the original implementation only sets the name attribute equal to $_[0] as well, by virtue of $self->{name} = shift. I was less interested in the case of empty array as I was in the case of setting the attribute 'name' to be a scalar value, which seemed to make more sense. Should have been more clear. ;-)

        As to die "No arguments in set_name()" unless @_;, note that I did leave a space in the setter to validate behavior, since we aren't going from a spec. Depending on the behavior required, that line may or may not be appropriate. The behavior you described as erratic is what I intended, but its interesting that we looked at the same OP and drew differing conclusions.

        I guess this is an example to designers about how unclear specs can lead to unexpected behavior. ;-)

        <-radiant.matrix->
        Larry Wall is Yoda: there is no try{} (ok, except in Perl6; way to ruin a joke, Larry! ;P)
        The Code that can be seen is not the true Code
        "In any sufficiently large group of people, most are idiots" - Kaa's Law

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://486123]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others meditating upon the Monastery: (5)
As of 2014-10-01 10:36 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    What is your favourite meta-syntactic variable name?














    Results (6 votes), past polls