Class attribute get/set approach flaws?

radiantmatrix has asked for the wisdom of the Perl Monks concerning the following question:

There's been a fair amount of banter about objects here lately, especially getters and setters. Now, I've usually been in the camp that using lvalue subs for this purpose is typically a bad idea (though it might be less so in Perl6). However, in meditating on the ideas and approaches in many PM threads on the topic, I thought of an approach which seems appealing.

Given my relative lack of wisdom in this topic area, I presume that I haven't been the first to think of it; yet, I never see it done. I assume that's because there is something wrong with it (or that there are simply unarguably better solutions). What I hope with this request is that my fellow Monks can help me understand if this really is a flawed approach, the reason(s) why, and what the superior approach is.

Without further ado, the code for a simple illustrive class:

package Objet; # yes, the 'c' is absent by design;
use strict;
use warnings;

sub new {
   my $self = bless {}, shift;
   my %args = @_;
   while (my $k, $v = each %args) {
      $self->attrib($k => $v);
   }
   return $self;
}

sub attrib {
   my $self = shift;
   my ($attr, $value) = @_;

   # if we aren't setting, then get.
   # I don't use defined $value, because setting to undef might be ok.
   return $self->{$attr} unless (scalar @_ >= 2); 

   # otherwise, set the attribute
   $self->{$attr} = $value;
}

1;
[download]

Of course, there's lots I'd do to validate that attributes should exist, and so on, but this is simple-case code. This might be implemented like:

use Objet;
my $o = Objet->new('aleph' => 1, 'gamma' => 2);

$o->attrib( 'zeta' => 900 ); # sets attribute 'zeta' to 900
print $o->attrib( 'aleph' ); # gets value of attrib 'aleph'
[download]

Update: it seems that I've managed to oversimplify to the extent that I've distracted from my own question. I already realize that there are major implmentation flaws in the above, but that's because it's basically simple psuedo-code that compiles. I realize that the attrib method would be more complex than it is: validation (including checking if an attribute exists {avoids typos}, and if an attribute is private/public), setting that is more complicated than an assignment, and so on would need to be addressed. A slight snippet of example:

sub new {
   my $self = bless {
     alpha => {
       value  => undef,
       set_ok => 1,
       get_ok => 1,
       set    => sub {
         # untested.
         my $self = shift;
         $self->_set_alpha(@_);
       },
       get    => sub {
         my $self = shift;
         $self->{alpha}{value};
       },
       valid  => sub {
         my $val = shift;
         return ($val =~ m/^\d+$/) ? 1 : 0; #only digits
       },
     },
   }, shift;

   return $self;
}   

sub attrib {
   my $self = shift;
   my ($attr, $value) = @_;
   
   die("No attribute '$attr'")
     unless (ref $self->{$attr} eq 'HASH');

   # set
   if (scalar @_ >= 2) {
      die("Not allowed to set '$attr'") unless $self->{$attr}{set_ok};
      die("Invalid value for '$attr'") 
        unless $self->{$attr}{valid}->($value);
      $self->{$attr}{set}->($value);
   }
   else {
      die("Not allowed to get '$attr'") unless $self->{$attr}{get_ok};
      return $self->{$attr}{get}->();
   }
}
[download]

Yes, there are still implementation issues, and I wouldn't implement new() that way at all, etc. But hopefully this makes it clear that there are plenty of ways to have independent setter/getter subroutines, but only one public interface to all of it, which is what I'm asking about.

So, what's wrong with this approach? Thanks in advance for hitting me with the Paddles of Enlightenment.

<-radiant.matrix->
A collection of thoughts and links from the minds of geeks
The Code that can be seen is not the true Code
"In any sufficiently large group of people, most are idiots" - Kaa's Law

Comment on Class attribute get/set approach flaws? Select or Download Code

Replies are listed 'Best First'.
Re: Class attribute get/set approach flaws? by sauoq (Abbot) on Nov 28, 2005 at 23:12 UTC
Using a single accessor for setting/getting all your attributes is a bad idea because you can't easily do any validation or implement behavior specific to a particular attribute. Say, for example, that you have an Employee class and you want to double check that an employee's salary isn't being set to a negative value or you want to update his benefits eligibility when his full_time attribute is set to true. -sauoq "My two cents aren't worth a dime.";	[reply]
Re^2: Class attribute get/set approach flaws? by Ovid (Cardinal) on Nov 28, 2005 at 23:26 UTC
Not necessarily true. It can actually make code easier to work with. my %validation_for = ( foo => qr/^\d+$/, # foo must be an integer bar => \&validate, }: sub attr { my ( $self, $attr ) = splice @_, 0, 2; croak "No such attribute $attr" unless exists $validation_for{$attr} +; return $self->{$attr} unless @_; # set the value my $validation = $validation_for{$attr}; my $new_val = shift; if ('Regexp' eq ref $validation) { croak "$new_val doesn't match $validation" if $new_val !~ $validation; } else { $self->$validation($new_val); # we'll assume it throws an error } $self->{$attr} = $new_val; return $self; } [download] With that quick hack, adding a new attribute is as simple as adding a new key/value pair to the validation lookup. You can then use a single method for getter/setters for everything and still have validation. Whether or not that's appropriate for your needs depends on your requirements. Also, "extra" behavior can easily be added to the validation subroutine, though at that point you'd probably want to rename the hash. Cheers, Ovid New address of my CGI Course.	[reply] [d/l]
Re^3: Class attribute get/set approach flaws? by sauoq (Abbot) on Nov 28, 2005 at 23:48 UTC
With that quick hack, adding a new attribute is as simple as adding a new key/value pair to the validation lookup. Well, that glosses over the fact that adding the key/value pair might not be easy depending on the validation and/or behavior you need for a given attribute... You might need to write a new validation function, for instance. And, you might want to change the names of things a bit if, for instance, you want functions that don't just perform validation but, say, set other values. (Change the radius and update the area, to use an example that's been used frequently today.) But, okay. Yeah. Sure. You can do things like that. I've even done it, though not quite like that. It might even buy you something in the common cases... I'm not sure how much though and I've come to think that just writing a separate method is easy enough and usually neater. So, that's what I usually do these days. (I do prefer accessors that both set and get though.) -sauoq "My two cents aren't worth a dime.";	[reply]
Re^4: Class attribute get/set approach flaws? by diotalevi (Canon) on Nov 28, 2005 at 23:54 UTC
Re^5: Class attribute get/set approach flaws? by sauoq (Abbot) on Nov 29, 2005 at 01:34 UTC
Some notes below your chosen depth have not been shown here
Re: Class attribute get/set approach flaws? by herveus (Prior) on Nov 29, 2005 at 00:24 UTC
Howdy! `use Objet; my $o = Objet->new('aleph' => 1, 'gamma' => 2); $o->attrib( 'zeta' => 900 ); # sets attribute 'zeta' to 900 print $o->attrib( 'aleph' ); # gets value of attrib 'aleph'` [download] One thing "wrong" with this approach is that you now have symbolic references, in effect. What if you misspell the name of the attribute? You have to some way to validate the attribute names at run-time (or, at least, you ought to). Using separate subs to be the setters and getters will give you a run-time exception if you misspell the attribute name. This can be seen as a better thing than quietly Doing The Wrong Thing. yours, Michael	[reply] [d/l]
Re^2: Class attribute get/set approach flaws? by sauoq (Abbot) on Nov 29, 2005 at 01:37 UTC
Well, in his defense he did say: "Of course, there's lots I'd do to validate that attributes should exist, and so on, but this is simple-case code." -sauoq "My two cents aren't worth a dime.";	[reply]
Re: Class attribute get/set approach flaws? by jonadab (Parson) on Nov 29, 2005 at 01:16 UTC
My take on this is that in general, as a rule of thumb, methods that don't do anything but get or set a variable are a sign of bad design. It would be different if there were some concrete reason for having methods get and set the variables. For instance, the setter methods of some classes might do validation, and that would provide a worthwhile excuse for their existance; if you think you might add validation in the future, you might go ahead and do the interface this way to ease that transition. Another example is Class::DBI, whose methods do rather more than merely get or set a plain old variable (and which, incidentally, implements both the Class::Value() and Class::get(value) interfaces; both are useful, the former for reasons pointed out by others in this thread, and the latter because sometimes it's terribly convenient to run a foreach loop through several values). Nevertheless, these are special cases, cases where the methods either currently do or someday might do something more than just get or set the value. Object-oriented design is a very useful approach, especially for certain types of problems, but (unless you're taking a class in it and need to make certain grades) don't get too caught up in textbook notions of "clean" OO design, wherein the professor will mark you down if any of your object's variables are public. If the methods are never going to do anything but get or set the object's values, they're probably superfluous and add needless complexity, which is usually a bad thing; in that case, it's probably better to let the calling code get or set the variables via normal assignment. This is especially true in Perl, because normal assignment has some flexibility that you might not think about and might not think to implement in your methods, e.g., it reacts appropriately to contexts, can work with array or hash slicing, and so on. It may also perform better than methods.	[reply]
Re^2: Class attribute get/set approach flaws? by sauoq (Abbot) on Nov 29, 2005 at 01:58 UTC
If the methods are never going to do anything but get or set the object's values, they're probably superfluous and add needless complexity, which is usually a bad thing; in that case, it's probably better to let the calling code get or set the variables via normal assignment. This is not likely to be a popular stance around here. I don't entirely disagree... This is useful for some very simple classes which are just glorified hashes anyway and, preferably, are used only in one application. But, you should recognize that there is a huge problem with this approach. It locks you into an implementation. Say you want to change your hash-based objects into "inside-out objects" down the road, for instance. You've got to change all the code that uses your class. If your class has gotten any real use, that's probably not going to be an easy task. So, unless you're pretty damn sure you're never going to change the implementation of your class, you don't want to do this. And, really, who's ever that sure? -sauoq "My two cents aren't worth a dime.";	[reply]
Re: Class attribute get/set approach flaws? by jonadab (Parson) on Nov 29, 2005 at 05:05 UTC
This is not likely to be a popular stance Popular opinion isn't everything it's cracked up to be. Popular opinion around here also favours CGI.pm, but I have very solid reasons for disliking it. Say you want to change your hash-based objects into "inside-out objects" down the road, for instance If I were going to make a change that substantial, I'd probably write a new class entirely and just declare the old one deprecated in favor of the new one. I'm all for code re-use up to a point, but trying to make a fundamental structural change like that to any non-trivial module is going to mean a trainload of headaches and subtler bugs than I really want to deal with if I can help it. Besides, while I do appreciate the usefulness of lexical closures (and, in fact, my one lone module on the CPAN accepts code references as a callback mechanism), using them for encapsulation in object-oriented code is a technique I reserve for the Obfuscated Code section (e.g., see this or this). However, I also neglected to finish explaining my original thought, i.e., that accessor methods can be a sign of poor design. This is not universally the case, but often it is better to accept named arguments when instantiating the object in the first place. Obviously you will sometimes have a value that needs to be changed later on by outside code for one reason or another, but if all of your variables have such accessors, the object basically is, semantically, a glorified hash. I am particularly leary of the "let's use AUTOLOAD to automatically generate get/set accessors for all of our variables" approach. If you are even tempted to do that, it's time to re-evaluate the entire design of the class, IMO.	[reply]
Re: Class attribute get/set approach flaws? by Perl Mouse (Chaplain) on Nov 29, 2005 at 10:54 UTC
The main reason I don't like this is because it makes inheritance awkward. If you are inheriting from another class that's using such an 'attrib' function as well, you have to know which attributes it's dealing with (it may for instance do validation), and you'll have to filter those and pass it to SUPER. With a long inheritance tree, this becomes awkward. Another reason I don't like it is that, as presented, anyone can set and inspect any attribute. That includes typoed attributes, or just someone being "nasty". And I fail to see the benefit for the user of this class. To me, the only advantage I can see is that the author of the module saves adding a few accessors - at the expensive of an inflexible, complicated, do-it-all routine. Good modules are optimized for the user, not the author. (Even if the same person performs both roles). `Perl --((8:>*`	[reply]
Re^2: Class attribute get/set approach flaws? by radiantmatrix (Parson) on Nov 29, 2005 at 16:00 UTC
Another reason I don't like it is that, as presented, anyone can set and inspect any attribute. That includes typoed attributes, or just someone being "nasty". Isn't this pretty much always true? Besides, I did mention that validation was a necessary part of implementing this concept: validation could easily include "you are(?:n't) allowed to modify this value". And I fail to see the benefit for the user of this class. To me, the only advantage I can see is that the author of the module saves adding a few accessors - at the expensive of an inflexible, complicated, do-it-all routine. Could you please expand on this? I guess I don't understand what is inflexible or complicated about the approach -- it seems simpler and more flexible from my (admittedly inexperienced) POV. Clearly, I'm missing something, and I'd like to better understand what it is. <-radiant.matrix-> A collection of thoughts and links from the minds of geeks The Code that can be seen is not the true Code "In any sufficiently large group of people, most are idiots" - Kaa's Law	[reply]