Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

Re^2: What CPAN modules are "good reads"?

by perrin (Chancellor)
on Jun 13, 2005 at 22:34 UTC ( [id://466309]=note: print w/replies, xml ) Need Help??


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

Catalyst contains some code that I consider pretty questionable -- things like undocumented abuses of import(), assumptions about the internal structure of other people's classes, etc. It's not all bad, but I wouldn't call it "clean."
  • Comment on Re^2: What CPAN modules are "good reads"?

Replies are listed 'Best First'.
Re^3: What CPAN modules are "good reads"?
by Hansen (Friar) on Jun 14, 2005 at 00:41 UTC

    undocumented abuses of import()

    The only place we use import() is when you declare which engine and plugins you want to use, but it is documented.

    assumptions about the internal structure of other people's classes

    We only make a assumption of your internal structure if you inherit from Catalyst::Base and don't overload the constructor, perfectly sensible IMO.

    etc

    It would be interesting to hear about the real cases, where we are unclean.

      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; }
      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); }
      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.

        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?"

        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;

        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.

Re^3: What CPAN modules are "good reads"?
by sri (Vicar) on Jun 14, 2005 at 00:17 UTC
    Feel free to discuss this on the Mailinglist.

    Isn't blind bashing under your level? This is everything, but not productive...
      Since Hansen asked about it here, I responded here.

      You're taking quite minor complaints about the source code far too seriously. I wouldn't even have bothered to mention these issues if the source hadn't been advanced as something to emulate. I certainly didn't mean it to sound like "bashing." Sorry if you were offended by my off-hand remarks.

      you don't take critics or comments that well, ey? as long as everyone is talking cheerful about Catalyst, all's well. But from the moment someone says something a little negative, you tell your age by the answers you give. :-p
      Catalyst may be cool, but don't overhype it ;-) If people hate it, don't try to convert them. It will only make things worse...
      btw: do really think perrin was bashing? :-|

      to ask a question is a moment of shame
      to remain ignorant is a lifelong shame

        No, i can take critics very well, but perrin's is pointless, see Hansen's comment.
Re^3: What CPAN modules are "good reads"?
by astroboy (Chaplain) on Jun 15, 2005 at 01:39 UTC
    I'm still waiting for a response to this. I'm all for reasoned debate, but you've made sweeping criticisms of Catalyst, without backing up your claims. No Open Source project is above criticism, but it should be measured and considered. Come on, show us some examples of "undocumented abuses of import(), assumptions about the internal structure of other people's classes." If you can't find any, then the developers don't deserve the slur on the project, and if you can then hopefully they'll work to make it better.
      Take it easy. I was at the beach, and not checking PerlMonks very often. It was also a rather minor criticism. I'm back now, and writing a reponse to Hansen.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://466309]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others imbibing at the Monastery: (3)
As of 2024-03-30 05:01 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found