in reply to Re^3: What CPAN modules are "good reads"?
in thread What CPAN modules are "good reads"?

You've done a lot of interesting stuff with Catalyst, and I'm not trying to insult your work. As I said, the issues with the code are not bad. They won't stop me from using Catalyst. They will stop me from recommending the source code to newbies as an example to be copied though, mostly because of how it strays from established perl practices.

The use of import() I'm referring to is in Catalyst.pm. The documentation says that some options can be passed when calling use Catalyst to load plugins and activate debugging. It doesn't say that if you don't call import the whole thing will blow up. I discovered this when I wanted to turn off debugging and changed the line in my auto-generated controller module from use Catalyst qw/-Debug/; to use Catalyst qw//;. Not importing anything is a common memory optimization technique under mod_perl.

I was curious about why skipping the import kills everything, so I looked at the code, and found this: 

    # Prepare inheritance
    unless ( $caller->isa($self) ) {
        no strict 'refs';
        push @{"$caller\::ISA"}, $self;
    }

[download]
Perl has well-established ways to set up inheritance. Messing with the caller's @ISA from your module's import() method is not a clean way to do it, and the documentation doesn't mention it being done at all. It does the same for the dispatcher class.

When I see no strict 'refs', it's a signal that something bad is about to happen. The uses here seem unncessary. There are other ways to keep track of a debug setting than adding a method to someone else's symbol table.

As long as we're talking about this code, I would also suggest breaking up this long method into smaller ones, and avoiding the use of UNIVERSAL::require to put a method into everyone else's namespace, but those are very minor things.

The other thing I referred to is not Catalyst::Base. I didn't look at that code. I was talking about Class::DBI::FromForm. It's arguably not directly a part of Catalyst, but it is a key component of Catalyst::Model::CDBI::CRUD and was written by sri, so the distinction is kind of academic.

While looking at how that module works, I found this code: 

sub _run_create {
    my ( $me, $class, $results ) = @_;
    my $them = bless {}, $class;
    my $cols = {};
    foreach my $col ( $them->columns('All') ) {
        $cols->{$col} = $results->valid($col);
    }
    return $class->create($cols);
}

[download]
This code blesses a hashref into the class of your Class::DBI module and calls a method on it. It assumes that any Class::DBI subclass will function happily as a hash with no data, and respond correctly to a columns() method call. It's a total violation of encapsulation and will break if Class::DBI changes its internals significantly in the future.

I'm not trying to make a big case out of these things. I don't think they are serious enough problems to merit a bug report, or even a complaint on the mailing list. They are certainly no worse than what I've seen in many other CPAN modules, and better than most. I do think they make the source a questionable example for young coders in training though. I hope you can see why and won't take it too personally.

Replies are listed 'Best First'.
Re^5: What CPAN modules are "good reads"?
by dragonchild (Archbishop) on Jun 16, 2005 at 12:59 UTC
    Perl has well-established ways to set up inheritance. Messing with the caller's @ISA from your module's import() method is not a clean way to do it, and the documentation doesn't mention it being done at all. It does the same for the dispatcher class.

    Your criticism is absolutely on point and this is one thing I found wrong with the module when I first started using it on Monday. However, it is documented, in a way. The only reason I know this is I think I've read every single document written about Catalyst twice since Monday.

    From the Tutorial:

    When the Catalyst module is imported by the application code, Catalyst performs the first stage of its initialization. This includes loading the appropriate Engine module for the environment in which the application is running, loading any plugins and ensuring that the calling module (the application module) inherits from Catalyst (which makes the Catalyst methods config and setup available to the application module). (Emphasis mine.)

    • In general, if you think something isn't in Perl, try it out, because it usually is. :-)
    • "What is the sound of Perl? Is it not the sound of a wall that people have stopped banging their heads against?"
[reply]
      Good memory! I'm sure you would agree that it should be documented in the Catalyst man page as well, not just in another tutorial document. When having trouble with a change in arguments to Catalyst, I looked there, not at the tutorial.
[reply]
Re^5: What CPAN modules are "good reads"?
by Hansen (Friar) on Jun 21, 2005 at 09:09 UTC

    Thank you for you detailed comments! I'll comment on those related to Catalyst core.

    Perl has well-established ways to set up inheritance. Messing with the caller's @ISA from your module's import() method is not a clean way to do it, and the documentation doesn't mention it being done at all. It does the same for the dispatcher class.

    There is nothing that stops you from setting up the application class manually, but by doing that you'll probably miss some very convenient features that Catalyst offers.

    Here is an example of doing it manually:

    package MyApp;

use strict;
use warnings;
use base qw[ Catalyst
             Catalyst::Plugin::Email
             Catalyst::Plugin::Static
             Catalyst::Dispatcher
             Catalyst::Engine::HTTP ];

MyApp->engine('Catalyst::Engine::HTTP');
MyApp->dispatcher('Catalyst::Dispatcher');
MyApp->log( Catalyst::Log->new );

MyApp->config(
    home => '/path/to/my/home',
    root => '/path/to/my/home/root'
);

MyApp->setup;

1;

    [download]

    Catalyst will automagically try to find the best Engine class for it's environment, so if you do it your self you'll have to create several application classes, one for each environment: development, testing and deployment.

    I agree that the documentation could be more clear about what import() is does.

    As long as we're talking about this code, I would also suggest breaking up this long method into smaller ones

    This has been on my TODO for a while, the import has now been broken down to five methods.

    and avoiding the use of UNIVERSAL::require to put a method into everyone else's namespace, but those are very minor things.

    This could be done, but i don't see the benefits of doing that. Using UNIVERSAL::require gives less and more readable code IMO.

[reply]
[d/l]
      You're right, the work being done by import() is pretty useful, but it could be done somewhere else -- maybe setup(), or a separate call. For example: 
      package MyApp;

use strict;
use warnings;
use base 'Catalyst';

MyApp->config(
    home => '/path/to/my/home',
    root => '/path/to/my/home/root'
);

MyApp->setup(qw/-Debug -Engine=HTTP Email Static/);

1;

      [download]
[reply]
[d/l]
        Muahaha...funny, you just found out how the Perl6 port of Catalyst handles this! :)
[reply]

        The above syntax will work with next release of Catalyst, in addition to the 'old' way.

[reply]

In Section Seekers of Perl Wisdom