Using a single method as both accessor and mutator isn't always bad, however this particular usage is because it probably isn't a documented behavior, and because the caller is mucking with "private" data in a way that violates the whole purpose of getters and setters (e.g., encapsulation)

If you want to use a single method for both purposes, it's better to do something like:

sub foo {
  my ($this, $arg) = @_;
  $this->{foo} = $arg if defined $arg;
  return $this->{foo};
}
[download]

As to the last point, accessors do serve to protect values, but in Perl unless you do some extra work you're really just depending upon the politeness of your caller to respect your boundaries. For serious encapsulation take a look at closures (Why are closures cool?, Why are closures cool, continued?).

Update: as per tantarbobus's correction.

Don't you to return $this->{foo} and not $this->foo.

Also, foo() breaks if you try to undefine $this->{foo} as in $this->foo(undef); so:

$test->foo("bob");
print "Bob:".$test->foo();
$test->foo(undef);
print "Undef:".$test->foo();
[download]

Will give:

Bob:bob
Undef:

Instead of checking for defined() maybe look @ the number of args that are passed into foo, like such:

sub foo {
  my ($this, $arg) = @_;
  $this->{foo} = $arg if @_>1;
  return $this->{foo};
}
[download]

Well, depends on how you want to go about signifying a 'null' value. Recall that an empty string is both defined and a common way of indicating that variable holds no useable value. So, calling the method like foo('') will effectively unset the variable. Of course in more complex situations (i.e., where the value to be set is an object reference, not just a string) you'd need something better, probably something along the lines of your suggestion to pass undef as a parameter.

I would opt for

sub foo {
    my $self = shift;
    return @_ ? $_ = shift : $_ for $self->{_foo};
}
[download]

Makeshifts last the longest.

Although the comment there says "accessors" those don't look anything like accessors. It looks like code to add members to an object rather than to retrieve or change the value of those members.

Is all of the code you pasted in the same module? If so, I'd say it isn't all that bad except for the fact that it is counterintuitive, inefficient and seems to come from a misunderstanding of encapsulation. This would probably be better:

push @{$self->{_fields}}, $field; 
$self->{_fields_hash}->{$field->fullname()} = $field;
[download]

If by "elsewhere" you mean somewhere not in the same module, then it's downright terrible.

-sauoq
"My two cents aren't worth a dime.";

It's halfway there. Perl has a tradition of using a get/set accessor of the same name versus other language having separate get_foo and set_foo methods. But this is a different matter, since the underlying values are not simple scalars. If you want to keep accessor-only property access, you should use something like:

 # Dereference so that they can't go behind our backs
 sub fields { return @{ $self->{_fields} } }

 sub add_field {
     my ( $self, $field ) = @_;
     push @{ $self->{_fields} }, $field;
     $self->{_fields_hash}->{ $field } = 
                       scalar @{ $self->{_fields} } - 1;
 }

 sub remove_field {
     my ( $self, $field ) = @_;
     my $idx = $self->field_exists( $field );
     return unless ( $idx );
     delete $self->{_fields_hash}->{ $field };
     splice( @{ $self->{_$fields} }, $idx, 1 );
 }

 sub field_exists {
     my ( $self, $field ) = @_;
     return $self->{_fields_hash}->{ $field };
 }
[download]

(Using the index in as a return value for field_exists() is kind of peek-a-boo logic, but it's a start.)

Untested because I have something due in a few minutes, but you get the idea :-)

Chris
M-x auto-bs-mode