views:

276

answers:

4

This may turn out to be an embarrassingly stupid question, but better than potentially creating embarrassingly stupid code. :-) This is an OO design question, really.

Let's say I have an object class 'Foos' that represents a set of dynamic configuration elements, which are obtained by querying a command on disk, 'mycrazyfoos -getconfig'. Let's say that there are two categories of behavior that I want 'Foos' objects to have:

  • Existing ones: one is, query ones that exist in the command output I just mentioned (/usr/bin/mycrazyfoos -getconfig`. Make modifications to existing ones via shelling out commands.

  • Create new ones that don't exist; new 'crazyfoos', using a complex set of /usr/bin/mycrazyfoos commands and parameters. Here I'm not really just querying, but actually running a bunch of system() commands. Affecting changes.

Here's my class structure:

Foos.pm

package Foos, which has a new($hashref->{name => 'myfooname',) constructor that takes a 'crazyfoo NAME' and then queries the existence of that NAME to see if it already exists (by shelling out and running the mycrazyfoos command above). If that crazyfoo already exists, return a Foos::Existing object. Any changes to this object requires shelling out, running commands and getting confirmation that everything ran okay.

If this is the way to go, then the new() constructor needs to have a test to see which subclass constructor to use (if that even makes sense in this context). Here are the subclasses:

Foos/Existing.pm

As mentioned above, this is for when a Foos object already exists.

Foos/Pending.pm

This is an object that will be created if, in the above, the 'crazyfoo NAME' doesn't actually exist. In this case, the new() constructor above will be checked for additional parameters, and it will go ahead and, when called using ->create() shell out using system() and create a new object... possibly returning an 'Existing' one...

OR

As I type this out, I am realizing it is perhaps it's better to have a single:

(an alternative arrangement)

Foos class, that has a

->new() that takes just a name

->create() that takes additional creation parameters

->delete(), ->change() and other params that affect ones that exist; that will have to just be checked dynamically.

So here we are, two main directions to go with this. I'm curious which would be the more intelligent way to go.

+4  A: 

In general it's a mistake (design-wise, not syntax-wise) for the new method to return anything but a new object. If you want to sometimes return an existing object, call that method something else, e.g. new_from_cache().

I also find it odd that you're splitting up this functionality (constructing a new object, and returning an existing one) not just into separate namespaces, but also different objects. So in general, you're closer with your second approach, but you can still have the main constructor (new) handle a variety of arguments:

package Foos;
use strict;
use warnings;

sub new
{
    my ($class, %args) = @_;

    if ($args{name})
    {
        # handle the name => value option
    }

    if ($args{some_other_option})
    {
        # ...
    }

    my $this = {
        # fill in any fields you need...
    };

    return bless $this, $class;
}

sub new_from_cache
{
    my ($class, %args) = @_;

    # check if the object already exists...

    # if not, create a new object
    return $class->new(%args);
}

Note: I don't want to complicate things while you're still learning, but you may also want to look at Moose, which takes care of a lot of the gory details of construction for you, and the definition of attributes and their accessors.

Ether
You're saying new() should *only* be used to create an object and perhaps set some of its attributes, but nothing else? I.e, place the 'does it already exist?' check outside of new(), and call it from outside of new(), so the class user would have to say` my $foo_obj = new Foos({name=>'Hithere'}); if ( $foo_obj->create({attr1=>'First',another=>'hm',again=>'thirdvalue'} ) { print "Success!";} else { print "Boo";} package Foo:sub create { my $self = shift; if ( }}`..?
Emmel
@Emmel: that's correct. If `new` returns something other than a newly-constructed object, that's a design smell IMHO. The *user* doesn't have to do the switch though -- you could have another `new_or_possibly_existing_from_cache` method which calls `new` if needed, or otherwise returns an existing object. Often this work is moved to a factory class.
Ether
+2  A: 

It is also a factory pattern (bad in Perl) if the object's constructor will return an instance blessed into more than one package.

I would create something like this. If the names exists than is_created is set to 1, otherwise it is set to 0.. I would merge the ::Pending, and ::Existing together, and if the object isn't created just put that into the default for the _object, the check happens lazily. Also, Foo->delete() and Foo->change() will defer to the instance in _object.

package Foo;
use Moose;

has 'name' => ( is => 'ro', isa => 'Str', required => 1 );
has 'is_created' => (
    is => 'ro'
    , isa => 'Bool'
    , init_arg => undef
    , default => sub {
        stuff_if_exists ? 1 : 0
    }
);

has '_object' => (
    isa => 'Object'
    , is => 'ro'
    , lazy => 1
    , init_arg => undef
    , default => sub {
        my $self = shift;
        $self->is_created
            ? Foo->new
            : Bar->new
    }
    , handles => [qw/delete change/]
);
Evan Carroll
Your linked article doesn't say anything about why Factory is bad in Perl, so I've moved the "(bad in Perl)" note outside of the link text. If you know of another article that goes into why Factory is bad in Perl, though, I'd love to see it.
Dave Sherohman
In most languages the problems stop at not knowing what type of object you're going to get back. With Perl the situation always seems worse, but it really depends on the design of the factory. Typically it is harder to subclass one of the classes that the factory instantiates because the factory does not permit this. Moreover, when the factory grows to permit something like this it often gets really messy. URI would be an example where it doesn't permit this (try sub-classing URI::http), and the DBI would be a good example of where this is annoying all drivers are statically typed in (DBI).
Evan Carroll
Not knowing what type of object you're going to get back is often the *purpose* of a factory.
darch
@Evan: your comment makes no sense. Well-written factories can do anything.
Ether
Re-read the comment, I give explicit examples. Name one functional factory implimetation in perl that doesn't suck. And, name one reason why factories are better than a single Moose class with traits (`MooseX::Traits`)...
Evan Carroll
+3  A: 

It is generally speaking a bad idea for a superclass to know about its subclasses, a principle which extends to construction.[1] If you need to decide at runtime what kind of object to create (and you do), create a fourth class to have just that job. This is one kind of "factory".

Having said that in answer to your nominal question, your problem as described does not seem to call for subclassing. In particular, you apparently are going to be treating the different classes of Foos differently depending on which concrete class they belong to. All you're really asking for is a unified way to instantiate two separate classes of objects.

So how's this suggestion[3]: Make Foos::Exists and Foos::Pending two separate and unrelated classes and provide (in Foos) a method that returns the appropriate one. Don't call it new; you're not making a new Foos.

If you want to unify the interfaces so that clients don't have to know which kind they're talking about, then we can talk subclassing (or better yet, delegation to a lazily-created and -updated Foos::Handle).

[1]: Explaining why this is true is a subject hefty enough for a book[2], but the short answer is that it creates a dependency cycle between the subclass (which depends on its superclass by definition) and the superclass (which is being made to depend on its subclass by a poor design decision).
[2]: Lakos, John. (1996). Large-scale C++ Software Design. Addison-Wesley.
[3]: Not a recommendation, since I can't get a good enough handle on your requirements to be sure I'm not shooting fish in a dark ocean.

darch
+1  A: 

Interesting answers! I am digesting it as I try out different things in code.

Well, I have another variation of the same question -- the same question, mind you, just a different problem to the same class:subclass creation issue!

This time:

This code is an interface to a command line that has a number of different complex options. I told you about /usr/bin/mycrazyfoos before, right? Well, what if I told you that that binary changes based on versions, and sometimes it completely changes its underlying options. And that this class we're writing, it has to be able to account for all of these things. The goal (or perhaps idea) is to do: (perhaps called FROM the Foos class we were discussing above):

Foos::Commandline, which has as subclasses different versions of the underlying '/usr/bin/mycrazyfoos' command.

Example:

 my $fcommandobj = new Foos::Commandline;
 my @raw_output_list = $fcommandobj->getlist();
 my $result_dance    = $fcommandobj->dance();

where 'getlist' and 'dance' are version-dependent. I thought about doing this:

 package Foos::Commandline;

 new (
    #Figure out some clever way to decide what version user has
    # (automagically)

    # And call appropriate subclass? Wait, you all are telling me this is bad OO:

    # if v1.0.1 (new Foos::Commandline::v1.0.1.....
    # else if v1.2 (new Foos::Commandline::v1.2....
    #etc

 }

then

 package Foos::Commandline::v1.0.1;

 sub getlist ( eval... system ("/usr/bin/mycrazyfoos", "-getlistbaby"
  # etc etc

and (different .pm files, in subdir of Foos/Commandline)

 package Foos::Commandline::v1.2;

 sub getlist ( eval...  system ("/usr/bin/mycrazyfoos", "-getlistohyeahrightheh"
  #etc

Make sense? I expressed in code what I'd like to do, but it just doesn't feel right, particularly in light of what was discussed in the above responses. What DOES feel right is that there should be a generic interface / superclass to Commandline... and that different versions should be able to override it. Right? Would appreciate a suggestion or two on that. Gracias.

Emmel