Method parameters validation and inheritance

[roman]

Dear monks,

this is not a serious problem, but I come across its variants quite frequently.

I have a method (say open_for_reading), which accepts hashref of parameters. Since it is quite important method I need all of its parameters to be understood. The open_for_reading method is overriden in a couple of subclasses, each of them understands more parameters.

my $context = $file->open_for_reading( {} );

my $context2 = $file->open_for_reading( { 'path' => $file } );
# OK
my $context_derived
    = $file_derived->open_for_reading( { 'path' => $file, 'join_longca
+lls' => 1 } );

# this should fail
my $context_derived = $file->open_for_reading( { 'join_longcalls' => 1
+ } );

Without the validation the code is something like:

package MyBaseClass;

sub open_for_reading {
    my ($this, $params_ref) = @_;

    $params_ref ||= {};
    return $this->get_context('path' => $params_ref->{'path'} || $this
+->path);
}

package MyDerivedClass;

sub open_for_reading {
    my ($this, $params_ref) = @_;

    $params_ref ||= {};
    my $context = $this->SUPER::open_for_reading();
    if ($params_ref->{'join_longcalls'}){
        ... modify context
    }
    return $context;
}

For parameter validation I found Params::Validate from CPAN. The only way I was able to add the validation is like this:

package MyBaseClass;

sub open_for_reading_validation_spec {
    return { 
        'path' => 0,
    };
}

sub open_for_reading {
    my ($this, $params_ref) = @_;

    $params_ref ||= {};
    Params::Validate::validate($params_ref, $this->open_for_reading_va
+lidation_spec);
    return $this->do_open_for_reading($params_ref);
}

sub do_open_for_reading {
    my ($this, $params_ref) = @_;

    return $this->get_context('path' => $params_ref->{'path'} || $this
+->path);
}


package MyDerivedClass;
use base qw(MyBaseClass);

sub do_open_for_reading {
    my ($this, $params_ref) = @_;

    my $context = $this->SUPER::do_open_for_reading($params_ref);
    if ($params_ref->{'join_longcalls'}){
        ... modify context
    }
    return $context;
}

sub open_for_reading_validation_spec {
    my ($this) = @_;

    return {
        %{$this->SUPER::open_for_reading_validation_spec},
        'join_longcalls' => 0,
    };
}

Instead of one method I have three:

• interface method which is not overridden in subclasses
• executive method which is overriden in subclasses
• method returning the validation specification

I can live with three methods, but besides open_for_reading there are also open_for_writing, open_for_processing, ... so whole interface suddenly "triplicates".

Do you think is there a different solution?

Even without the validation there is a "naming" problem of its own. I have an interface method which does initialization and cleanup, and between these two blocks is a code to be overriden in subclasses.

sub method {
    my $this = shift;
    
    $this->do_some_initialization;
    $this->do_something_which_can_be_overriden_in_subclasses();
    $this->do_some_cleanup;
}

I put the inner block into separate method, but I often have problem to name it. Do you use some naming convention for the "executive" methods? Prefixes (do_, exec_)? Postfixes (_body)?

[kyle]

I would probably try to structure my code so as to avoid the use of $self->SUPER::whatever(). Then I wouldn't need to have a separate method (that subclasses override) to store the spec for Params::Validate.

You can still pull the spec out of the method itself using lexicals and closures.

Read more... (889 Bytes)

If you have a bunch of stuff in open_for_reading, decompose it into private methods that the main open_for_reading then calls. The overridden open_for_reading in the child class can then call the same methods (or some subset of them). They enforce their own parameters, and callers will have to abide by them but won't have to have their own copy of them.

Since we're not talking about real code here, I'm necessarily doing a lot of handwaving. Please let me know if I'm not getting my point across.

As for part two of your question, I'd probably do this:

sub method {
    my $self = shift;

    $self->method_init;
    $self->_method;
    $self->method_cleanup;
}

If you don't like the "_method" naming, you might pick another suffix ("_exec", maybe). I'd prefer a standard set of suffixes over a standard set of prefixes.

Another approach would be to make that group of methods into an object. It can do its initialization during construction, cleanup in DESTROY, and the method itself can keep its name. Then you subclass it and override that one method. I don't think that's always a good idea, but it definitely takes the clutter off somewhere else, and that's often a good thing.

[Herkum]

In referen to naming conventions, I have a simple rule, make it simple to read, it will be simple to program. This basically means,

1. Avoid abbreviating. This is not the 'Wheel of Fortune' you can spell out whole words.
2. Avoid writing sentences. You should be able to name a method to encapsulate its utility in no more than 3 words. Anything more it start to sound like a government agency.
3. Name your method within the context it is being used. You know what subject you are talking about, don't be verbose about it.

Taking the example of some of your code above,

my $context = $file->open_for_reading( {} );

## Change to
#############################
my $context = $file->reading;

my $context_derived
    = $file_derived->open_for_reading( { 'path' => $file, 'join_longca
+lls' => 1 } );

What the hell is 'join_longcalls'? For some reason it sounds like a pirate, and it is basically incomprehensible so it might was well be a pirate. Make it read like you actually write, don't make something up and hope people understand what the hell you are talking about.

[TGI]

This must that "pirate software" I keep hearing about!

Really, there's no reason to stop there!

use Acme::Lingua::Pirate::Perl;

...

my $context_derived = $file_derived->open_for_reading( { 
    'path' => $file, 
    'join_longcalls' => 1 
} );

scuttle her matey "Avast! Context error - " $file_derived->error
    if $context_derived be '' curse ye deadlights;

Of course, roman may prefer other dialects.

Time for a brief lapse into serious commentary, and nitpicking. The problem I see with "join_longcalls" is that isn't "join_long_calls". Consistent formatting of parameter names, variables and function/method names is important. Otherwise you wind up with PHP.

[roman]

Thanks for your comments. I should probably give some longcalls explanation.

Code sample given is a simplified but real world example: $file encapsulates a binary file (log) produced by a phone switch. It contains cdrs (Call Data Records) describing voice calls carried out. The file is used as a source for several target applications (billing, dwh).

If a call continues longer than say half an hour it is logged by parts. If join_longcalls is set for open_for_reading only complete calls are produced by read_cdr and the total duration of the call is summed up.

So the join_longcalls flag really causes the long calls to be joined. In such case I am not sure whether to use join_long_calls or joins_long_calls?

I think open_for_reading is clearer than plain reading since I try to use imperative for my subs.

As kyle suggested, I could avoid some of the "SUPERs" and turns the code into:

my $context = $file->open_for_reading({'path' => $path});
$context->joins_long_calls(1); 
# or better like this, 
# because there is no property join_long_calls
$context->join_long_calls;

The open_for_reading, actually returns Class::Prototyped based object, so join_longcalls - whatever its implementation is - actually wraps read_cdr method.

This approach has some advantages:

• The interface of open_for_reading remains same across classes.
• It is more self checking - since the parameters were turned into method calls, which fail when called but not defined.

This approach has also some disadvatages:

• Although $lc->join_long_calls can be called anytime, it make sense only immediately after open.
• When I have target file (representation of the billing, dwh file) I could have previously used a declaratory source_params method containing the parameters to be passed to open_for_reading, now I had to redefine some open_source method.

[GrandFather]

Using attributes may provide another avenue to explore. I had a brief play with them for this sort of thing in Attributes with new to manage parameters

[talexb]

Interesting problem. If I understand the situation correctly, you have a base new method that has a few parameters, and a sub-class new method that has a few more parameters.

I would probably approach this in the sub-class new method by extracting the sub-class parameters from the hash, reserving them, calling the super-class' new method with just the parameters it understands, and then continue processing with the previously reserved parameters, operating on the new object. The destroy method may also have to do some destroy local and destroy super work.

In keeping with OO precepts, you want to make this process invisible to the base class.