Conditional inheritance strategy

[citromatik]

Hi all

I have a class that can create an object using data coming from different sources / formats. Like:

package My

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

   if ($args{source} eq 'xml') {
      require My::XML;
      @ISA = qw/My::XML/;
   } elsif ($args{source} eq 'json') {
      require My::JSON;
      @ISA = qw/My::JSON/;
   }
   $self->_init(); # Versions in both My::XML and My::JSON

   return ($self);
}
# rest of methods
sub method1 { ... }
sub method2 { ... }

The packages My::XML and My::JSON:

package My::XML;

sub _init { ... }
1;

package My::JSON;

sub _init { ... }
1;

The calling code would be:

use My;

my $obj = My->new(source=>"xml");
# Do whatever with $obj

Is this a correct design to face this? Any caveat you may anticipate? Are there better alternatives?

Thank you very much in advance for your help

[Corion]

Why have package My:: inherit from either My::XML or My::JSON at all? I would have My::new() return either a My::XML or My::JSON. Maybe both, My::XML and My::JSON can inherit from My::Base if they really share common routines.

[citromatik]

Thanks a lot for your thoughts

In fact I followed your last suggestion in a first version of the classes (My::XML and My::JSON inherited from My::Base). But I found that doing it in that way you emphasize how the data is stored (XML or JSON in this case). But in both cases you should be able to end up with the same object regardless how the information to build it was stored. I mean, it should be natural to get an object of class My instead of My::XML or My::JSON.

I don't know if this has any sense.

[morgon]

What you are doing is basically a factory design pattern where the factory (in your case the My:: package) usually is not part of the hierarchie of the classes it creates.

I mean, it should be natural to get an object of class My instead of My::XML or My::JSON.

I am not sure if I understand your question, but clients of your factory would usually not bother weather they have a XML or a JSON instance but work through the interface that the base-class (My::Base) provides.

[Corion]

Maybe you want then My->from_xml and/or My->from_json? Why is it important that My objects are XML or JSON objects? It seems to me that they should maybe be able to display or store themselves as XML or JSON, which I would put into a separate plugin or serialization class.

[JavaFan]

Is this a correct design to face this?

Not in general. @ISA is a package variable, it isn't bound to a specific instance. So, if you do:

my $x = My->new(source=>"xml");
my $y = My->new(source=>"json");

then now both $x and $y are instances of a class that inherits from My::JSON. If My::XML and My::JSON only contain _init, and this is only called from new, you're getting away with it, but if they also contain methods that will be called elsewhere from the program, behaviour may be unexpected.

Perhaps you can reverse the inheritance order:

package My::XML;
use My;
our @ISA = qw[My];

sub init {...}

1;

package My::JSON;
use My;
our @ISA = qw[My];

sub init {...}

1;

package My;

sub new {
    my $class = shift;
    my $self = bless {}, $class;
    $self->init;
    $self;
}

1;

package main;
use My::XML;
use My::JSON;

my $x = My::XML->new;
my $y = My::JSON->new;

[citromatik]

Not in general. @ISA is a package variable, it isn't bound to a specific instance. So, if you do:

my $x = My->new(source=>"xml");
my $y = My->new(source=>"json");

then now both $x and $y are instances of a class that inherits from My::JSON.

Good point

If My::XML and My::JSON only contain _init, and this is only called from new, you're getting away with it, but if they also contain methods that will be called elsewhere from the program, behaviour may be unexpected.

Yes, My::XML and My::JSON only contain _init and this is only called from new, so this is not a problem, but I think that I will reverse the inheritance as suggested several times in this thread

Thanks

[bart]

That looks fine to me. There are similar mechanisms in use on CPAN, such as:
• Digest
• File::Spec
You could check how they did things, as a source of inspiration.

One caveat: you don't have any warning if the "source" isn't recognized.

[BrowserUk]

Hm. It's not possible to tell from the posted snippets whether this really makes sense.

My instinct tells me that the XML or JSON is only used to initialise the objects of class My, and that once so initialised, the instances from either source are essentially identical. If so, then I would deem what you are doing as "unnecessary inheritance".

Based upon the above assumption, I would suggest that by using inheritance here, you are loading--and persisting--the overheads of the XML or JSON classes into each instance of My class for no good reason. That is, you are forcing each instance of My class to carry around the baggage--instance data and class references--of whichever base class was used to initialise it. And for no good reason, as that baggage will never be used again during the life of the My class instance.

If so, if you instantiated an instance of the relevant XML or JSON class to just parse the source data; then retrieved the data required by My class; and then allowed the parse instance to be reclaimed as soon as the initialisation of a new My class instance was complete; then your My class instances might be individually smaller. And possibly more efficien,t because of shorter method resolution paths.

Hard to tell from what you've posted, but worth considering.

[citromatik]

Hi BrowserUk,

You are right in your assumptions. My::XML and My::JSON only provide data to the inherited class, no other methods are used (except methods internally used by My::XML or My::JSON).

If so, if you instantiated an instance of the relevant XML or JSON class to just parse the source data; then retrieved the data required by My class; and then allowed the parse instance to be reclaimed as soon as the initialisation of a new My class instance was complete; then your My class instances might be individually smaller. And possibly more efficien,t because of shorter method resolution paths.

Sorry if I misunderstand your. Are you suggesting the following?:

Package My:

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

   my $data;
   if ($args{source} eq 'xml') {
      require My::XML;
      # @ISA = qw/My::XML/;
      $data = My::XML->new(%args);
   } elsif ($args{source} eq 'json') {
      require My::JSON;
      # @ISA = qw/My::JSON/;
      $data = My::JSON->new(%args);
   }

   my $self = bless \%{$data}, $class;
   return ($self);
}
# rest of methods
sub method1 { ... }
sub method2 { ... }

1;

Package My::XML

package My::XML;

use XML::Simple qw(:strict);

sub new
{
   my ($class,%args) = @_;
   my $self = bless {}, $class;
   $self->_init();
   return $self;
}

sub _init { # Code to read the data from XML source }

1;

Same for My::JSON

Thank you very much for your comments

[BrowserUk]

Are you suggesting the following?: [ ... CODE ... ]

Yes, that is pretty much what I was suggesting--but only because I assumed that the XML & JSON modules contained classes. Ie. That there was a purpose in inheriting from them because they had local state and more methods than just _init().

With your confirmation that they only contain one _init() subroutine each--although you are calling them as methods--personally, I can see no reason not to simply move the two _init() routines into your My package (with different names), and drop the extra packages.

[JavaFan]

Based upon the above assumption, I would suggest that by using inheritance here, you are loading--and persisting--the overheads of the XML or JSON classes into each instance of My class for no good reason. That is, you are forcing each instance of My class to carry around the baggage--instance data and class references--of whichever base class was used to initialise it. And for no good reason, as that baggage will never be used again during the life of the My class instance.

I wonder what baggage you are referring to. I would presume to point of _init is to set up instance data - why else call a method named _init from new? Of course, if said data is never used, it's pointless, but the code fragment doesn't suggest so. As for a class reference, any object will have a class reference - a single string (package name). The reference doesn't increase depending whether inheritance is used or not. But even if I misunderstood you, your arguments seem to be against using inheritance in general, and not specific to the OP.

[BrowserUk]

If nothing else, having the initialisation class in the inheritance tree via @ISA, is baggage, if there are no other methods than _init(). Looking back at the OPs code, there seems to be little reason for calling the _init() routines as methods rather than subroutines.

Indeed, as no instance of the initialisation classes is ever instantiated, it could equally be termed "poor OO". And it is a weird use of "inheritance" in every sense. If the idea is simple to provide two methods of initialisation, better I would say to have them both in the main My package space called (say) _initFromXML() and _initFromJSON() and ditch the extra package spaces entirely.

But, as I said, it is quite hard to tell from what the OP posted, as he has obviously simplified the code for the purpose of the question. It is always possible that he also has code in each base package to persist the objects to disk or database, which might make all the difference.

[kcott]

Basically this looks good. Here's a few thoughts and suggestions:

• Your if-elsif has no else. In this case, handling missing or unknown source (as alluded to by bart) would be appropriate.
• Consider making the source types case-insensitive.
• Do you need to pass %args to _init()?
• You may find it useful to have an _init() in My which calls $self->SUPER::_init().
• Is there a reason to return $self in list context?

[citromatik]

Thanks for your answer.

As for your specific suggestions, the code I posted is a simplification of the actual code that I have (where most of your thoughts have been addressed). I was interested in possible issues using the strategy.

Thanks again

[jethro]

If I get you correctly, you want each object either to be inherited from My::XML or My::JSON, but that doesn't work, as inheritance is a configuration at the class level

One solution would be to store a reference to an object of type My::XML or My::JSON into your objects and call methods from there.

Or you could simply have two classes of type My::XML and My::JSON that both inherit from My

[ikegami]

Don't have My inherit from My::XML and My::JSON, have My::XML and My::JSON inherit from My.

I'm guessing you want to do this since you get a file name and you want to load the file using the right module.

package My;

sub load {
   my ($class, %args) = @_;
   my $file_type = ...;
   if ($file_type eq 'xml') {
      require My::XML;
      return My::XML->new(%args);
   } elsif ($file_type eq 'json') {
      require My::JSON;
      return My::JSON->new(%args);
   }
}

sub new     { ... }
sub method1 { ... }
sub method2 { ... }

[roboticus]

citromatik:

It sounds as if the subclasses are simply artifacts of the format of the data source, and wouldn't have any behavioral differences otherwise. If that's the case, I wouldn't make the objects different types. Instead, I'd provide reader classes that read My objects, like My::Reader::XML.

...roboticus

Update: Consider this a "me, too" node, as I missed BrowserUk's node above.

[mojotoad]

You have a view: standard methods for your object flavor

You have a control: what you do with said well-behaved objects

You have a model: how stuff is retrieved and/or stored -- in this case XML or JSON or whatever, summon the demon from the depths and make it dance. DANCE I say.

Yes, it's true that purists will object to my interpretation, but they drank the cool-aid. I drank the cup.

Cheers,
Matt

[happy.barney]

few notes:
use dispatch map

our $dispatch_map = {
  xml => 'My::XML',
  json => 'My::JSON',
};

sub new {
 ...

  die unless exists $dispatch_map->{$args{source}};

  my $class = $dispatch_map->{$args{source}}
  Module::Load::load ($class);

  $self->{engine} = $class; # or $class->new ($self);
}

you can call foreign methods ...

  $self->My::JSON::_init;

  # or

  my $method = $self->{engine} . '::_init';
  $self->$method;

[pajout]

Answer 1: Why not, if it does work for you...

Answer 2: Perhaps I am too rigid, but I think that ascendant would not know anything about it's descendants.