Help seperating business logic from controller (catalyst/dbic)

[uG]

I was reading a new interview with mst today about the moose port of catalyst, and caught a few things he mentioned that I (as a new catalyst/mvc user) don't think i'm doing. The piece of code i'm including basically checks to see if a form has been filled out, and if so does some stuff. Otherwise, it shows the form that needs to be filled out.

sub create :Local{
   my ( $self, $c ) = @_;

   my $nation = $c->request->params->{nation};

   #Action Logic; Form was submitted, lets create then
   #Check to see if the nation name already exists
   if($nation) {
       unless($c->model('DB::Nation')->find({name => $nation})) {
               #If not, create the new nation!
               $c->user->nations->create({
                   name  => $nation,
                   owner => $c->user->get('id'),
                   map_nation_resource => [ { resource_id => 1 }, { re
+source_id => 2 } ],
               });

               $c->redirect($c->uri_for('/nation'));
       }
       else {
               $c->stash->{error_msg} = "Nation name already taken!";
       }
   }

      return $c->stash->{template} = 'nation/create.tt2';
}

Really i'm just still wet behind the ears and want to be sure i'm understanding these mvc techniques :) EDIT: I'm basically just asking if how I handled that action is good practice or not.

[stonecolddevin]

That looks like a mighty fine controller method to me.

Is there something specific you're looking for help with concerning Catalyst and MVC?

[uG]

<mst> ...everything that's business logic rather than UI flow logic should live in the model. Write methods in your DBIC ->table classes. Use load_namespaces and write methods in your resultset classes. If there's an extra layer of logic that needs to be in front, then -that- should be a model that's exposed to your app. This way everything can be re-used in batch jobs, cron scripts, command line tools, etc...

It was this tidbit that triggered my post. But perhaps i'm just confused with the difference between business and ui flow logic.

[perrin]

There are many ways to interpret MVC in a web context. One of them is to say that all of the actual form processing should be handled in some kind of "action" classes, separate from the controller, which you would then say are a part of the model. (Does it make sense to put these in DBIC table classes? Only if the action is reasonably a method of the object type that class is representing. Otherwise, a separate action class makes more sense.)

Another way to think about it is to say that the "controller" classes in a web framework are already abstract from the web itself and call Catalyst itself the controller and your controller class part of the model. This is a practical point of view and by far the most popular way to do things. However, things like the redirect in your example are obviously web-specific.

The point of all this is separation of concerns and code reuse. Ask yourself, is there any business logic in here that I might want to use from a non-web context, like a cron job? If I did want to, could I do it?

In the example you show here, it fails that test. You have rules expressed here (e.g. you can't create a nation with a name that is already taken) which can't be reused outside of this web context. To solve this, you could create a method (maybe in DB::Nation, maybe in Action::NationManager, etc.) which takes the relevant parameters (name, user, etc.) and either returns a new nation or throws an exception. Your controller code calls it and handles the exception by redirecting.

Sound like a lot of work? It can be. It's definitely worth doing in situations where the code would genuinely be reused, but debatable otherwise. It's up to you to find the sweet spot on the purity <--> practicality continuum for your application.

[uG]

Thanks to the advice I have received, I have made the following modifications which I will include for future reference to anyone who may have had the same questions as me... thanks dhoss and perrin!

Controller

sub create :Local{
   my ( $self, $c ) = @_;

   my $nation = $c->request->params->{nation};

   #Action Logic; Form was submitted, lets create then
   #Check to see if the nation name already exists
   #UPDATED: create method overloaded to return 0 if
   #nation name exists

   if($nation) {
       if($c->user->nations->create({
                                     name  => $nation,
                                     owner => $c->user->get('id'),
                                     map_nation_resource => [ { resour
+ce_id => 1 }, { resource_id => 2 } ],
                                     })){
               $c->redirect($c->uri_for('/nation'));
       }
       else {
               $c->stash->{error_msg} = "Nation name already taken!";
       }
   }

      return $c->stash->{template} = 'nation/create.tt2';
}


ResultSet subclass

package Game::DB::ResultSet::Nation;
use strict;
use warnings;
use base 'DBIx::Class::ResultSet';

#Override nation's create method to check if nation name exists
#Before creating. If it does, return 0

sub create {
    my $self = shift;
    my $nation) = shift;

    if($self->find( {name => $nation->{name}} ) ) {
        return 0;
    }

    $self->next::method($nation);
}

1;