All the Perl that's Practical to Extract and Report

OO Newbie Mistake #23

Ovid on 2006-09-29T11:45:51

If you're doing this:

$self->validate( $self->user );

You've probably made a mistake. Don't pass methods things they can figure out for themselves, m'kay?

Hmmm.

Abigail on 2006-09-29T15:12:54

Is that really a mistake? Seems to me that 'validate' is a function that validates strings. It just so happens that in this call it needs to validate the result of a method that was called on the same object.

It's a trade-off. If you write 'validate' such that it doesn't take argument, but calls 'user' on its first parameter, you have a function with a simpler interface. OTOH, the function is less general than it can be - all it can do is validate its own user. It also should have been named differently, for instance 'validate_user'.

Now, for the program you lifted this statement of, it might have made more sense to have a method that just calls 'user' on its first parameter. But if all I'm shown is the given statement, I won't judge it to be right or wrong.

Re:Hmmm.

Ovid on 2006-09-29T16:26:31

I shouldn't have called it 'validate'. I guess that was a red herring. My boss has given me permission to sometimes post code snippets so long as I make it clear it's code being cleaned up and that I don't reveal any business knowledge. As a result, I really can't post more context. In this case, it was closer to something like this (still anonymous, but closer in intent):

sub _post_to_website {
    my ($self, $user) = @_;
    my $lwp = LWP::UserAgent->new;
    ...
}

# and the actual call
$self->_post_to_website( $self->{username} ); # reaching into the object

There was no validation of the username. The username is required to be the object's username in order for the POST to succeed.

Re:Hmmm.

markjugg on 2006-09-30T00:37:42

I agree with Abigail. Explicit argument passing makes it clearer what happening, and can make the code more re-usable.

A methodology of sticking everything thing in the object and then referencing it there is a close cousin to the evilness that is global variables.

However, if you code is full of $self->user always being passed to $self->validate.... there could be a problem.

A counter-example ...

drhyde on 2006-10-03T13:46:33

Consider the case where you usually want to validate something (eg whether a user has permission to access a file) for the current user, but sometimes for some other user, like in File::Find::Rule::Permissions.

This means that at least sometimes you have to pass in that parameter. For the sake of making the parameter list easier to parse, or to avoid confusing maintenance programmers by having the same method take two different forms of its arguments, I think I would at least consider always passing in the username.

I prefer to pass a hash of parameters thus faking up named parameters. That avoids both of the problems above. But using named parameters is a coding style choice that might not suit all cases.