views:

240

answers:

6

I've been discussing a code style issue with a friend. We have a series of packages that implement an interface by returning a specific type of value via a named subroutine. For example:

package Foo::Type::Bar;
sub generate_foo {
    # about 5-100 lines of code
    return stuff here;
}

So you can go:

my $bar_foo = Foo::Type::Bar->generate_foo;
my $baz_foo = Foo::Type::Baz->generate_foo;

We have many of these, all under the same Foo::Type::* hierarchy.

I think the packages should clearly indicate that they implement the foo_generate interface, e.g.:

package Foo::Type::Bar;
use base 'Foo::Type';
sub generate_foo {
    ...
    return stuff here;
}

I think this is good code style, much more clear and clean for other coders exploring the code. It also lets you check Foo::Type::Bar->isa('Foo::Type') to see if it implements the interface (other parts of the subsystem are entirely OO).

My friend disagrees. Some arguments he makes are:

  • Foo::Type::* packages are clearly named, and used only in an internal project, and therefore there's no question of wondering whether or not a given package implements an interface
  • the packages are often small and part of a standalone subsystem, and they feel to him like batch files or conf files, not heavy Perl OO code
  • Perl expresses implementation via inheritance, which may be complex or problematic, particularly when one gets to multiple inheritance
  • adding a Foo::Type superclass doesn't add any value, as it would literally be an empty package, used only to enable ->isa lookups
  • programmatically indicating interface implementation is a matter of personal code style

Is one or the other of us "right"? What would you do?

Edit: in examples, renamed Foo::Generator to Foo::Type

+1  A: 

In my opinion, it depends on scale and is very subjective. The larger a system becomes, the more advantageous it is to build-up the meta data around it. If you expect it to grow, it's possible these usage scenarios change and will make it worth the time to implement. But, perl is generally geared toward pragmatic programming, code only what's needed and nothing more. In your case, knowing what little i know, i'd say leave it as is. If this were java, i'd tend to lean the other direction since java benefits far more from interface usage from the start.

nicerobot
+1  A: 

In keeping with pragmatical spirit of Perl, I would only add base class if it was valuable by itself. Adding a collection of empty functions is not much fun.

On the other hand, the base class can be useful as a place to keep your POD. And to define some constants... Do you have Foo::Type.pm? If you do, and discuss generate_foo there, then there is no need for Generator.pm...

Yes, it's a judgment call.

Arkadiy
+1  A: 

Generally speaking, if you have an "empty" level in your inheritance hierarchy, either something should be in it that you haven't factored out yet, or, if there really is nothing to put in it, it shouldn't be there at all. I think this is the source of your friend's discomfort with the Foo::Type class - there's no reason it has to be there; it just is, as you currently describe the code.

Levels beyond the first in your class hierarchy should serve to differentiate multiple things at that level. Otherwise, people will look for a reason for the level to be there, even if there is none: I might suggest actually eliminating the middle level altogether. The fact that it has a name that is semantically null (Type) indicates that it's not really needed. Perhaps there's a discrepancy in your thinking about what the objects are and what they are supposed to be doing versus your naming convention.

I might argue that

Foo::Bar->generate
Foo::Baz->generate

makes more sense if the objects returned depend more on their Bar-ness or Baz-ness to describe them than on their Type-ness.

A really good way to evaluate stuff like this is to say to yourself, "Would I put it up on CPAN this way? Would I want (potentially) any Perl programmer in the world to see this?" If you have any discomfort with that (a completely empty class? Hmmm.) then you should step back and reconsider.

Joe McMahon
+3  A: 

Well, there's "duck-typing" that other dynamic languages use. UNIVERSAL::can accomplishes this to the degree that other languages tend to use it.

$unknown->doodle() if $unknown->can( 'doodle' );

The name "duck" typing comes from the idea that "if it walks like a duck, and talks like a duck... it's a duck". However, we're not seeing if it walks "like a duck", we're just seeing if it does something somebody called "walk" because that's what we call what a duck does too. A Tree Walker object, for example, does not "walk" like a duck!

You wouldn't want to pass the wrong object certain things.

$path_or_OS_I_dont_know_which->format( "c:", 'y' );

So how well duck-typing works depends on your expectations on the other side. Plus it tends to get messy if the contract is more complex. You just multiply uncertainty--though narrowing out objects by sheer unlikely hood that two classes would accidentally have 23 like-named methods.

if ( $duck->can( 'talk' ) && $duck->can( 'walk' )) { 
    $duck->walk();
    $duck->talk();
}

that's fine, but

if ( $cand->can( 'walk' ) && $cand->can( 'talk' ) && ... && $cand->can( 'gargle' )) {
    ...
}

gets a little silly. Plus, some of these methods might really be optional, and so you would have to intersperse it with:

$cand->snicker() if $cand->can( 'snicker' );

(Of course junctions would clean it up a little bit.)

And it can get really complex with intermediate products of methods you have to pass to some other method which also might be there.

Compatible

I have a lightweight pattern I call 'Compatible'. There is no actual package called this, but if a class implements what Date does then it puts it in @ISA;

our @ISA = qw<... Date::Compatible ...>;

Thus there are only two isas that need to be called--and I encapsulate it in an is_one sub, anyway:

sub UNIVERSAL::is_one { 
    my ( $self, @args ) = @_;
    foreach my $pkg ( @args ) { 
        return 1 if $self->isa( $pkg );
    }
    return 0;
}

sub UNIVERSAL::is_compatible_with {
    my ( $self, $class ) = @_;
    return $self->is_one( $class, "${class}::Compatible" );
}

It at least gives things a contract to fulfill, and for inquiring code to test against. The implementation can still decide how important it is to fulfill a particular method of the contract, if it's not picky.

Axeman
+2  A: 

What I really want to know is why you're talking about

my $bar_foo = Foo::Type::Bar->generate_foo

rather than

my $bar_foo = Foo::Type::Bar::generate_foo

In the first case, there's a thin veneer of OO, which suggests that some sort of OO / inheritance might be on the cards; but looking at your colleague's objections to adding explicit inheritance from an empty class, it seems pretty clear that generate_foo is being called in a non-OO fashion, and that no inheritance is expected.

With that in mind, nobody will ever type

if (Foo::Type::Bar->isa('Foo::Type')) { ... }

immediately before

my $bar_foo = Foo::Type::Bar::generate_foo;

as they can tell pretty quickly whether Foo::Type::Bar has a generate_foo function, and they'll determine that when they're writing their code. It looks like you're looking for some kind of "we're doing the right thing" assertions, when in fact if a bit of code gets that assertion wrong it'll crash the first time it's run.

It looks like you're dealing with non-OO code, in which case I wouldn't apply standard OO methods to the problem. Perhaps look at Joel Spolsky's article on why Hungarian Notation can be OK, done properly. If you make it obvious if code is right or wrong, you don't have to worry about hand-holding or enforcing little-needed standards.

Sam Kington
+1  A: 

I think you should upgrade to Moose if you are asking these questions. There you'll be able to define your interface by creating a role or class with appropriate abstract methods.

I agree with you that doing this can add valuable information. In Java there is the concept of a "marker interface," which is an empty interface which has no methods which exists only to tag a set of classes as being useful for a certain purpose. The most common example of this is the Serializable interface.

Of course, just because it can be useful does not mean it will be in your particular situation. :)

skiphoppy