Ovid

I haven't updated about Veure in a while and though this post isn't really about Veure, per se, I'll use some code from it to illustrate a "thin controller."

There's a lot of confusion about the thin controller/fat model advice which gets passed around. In fact, I've seen some developers get upset about the idea, claiming that it's the model which should be as thin as possible. I'll explain what's really going on and give some real-world examples, using code from Veure.

Here's a common thing I see in test suites:

my $mech = $test->get_mechanize;
$mech->get_ok(
    '/path/i/want/to/test',
    'I should be able to fetch my stuff'
);

And then the test fails with a 500, dumping $mech->content is painful and doesn't show the actual error, and you break out the debugger or print statements. After 15 minutes of messing around you discover that your $self->user wasn't returning anything because you forgot to have your $mech object log in, but if you had just seen the error in the first place, you would have known instantly what was going on.

Yeah, having that full integration test is nice, but it can really add some overhead.

Plus, it's often slow. Very slow. But you don't really have a choice because someone has packed so much logic into the controller that there's no easy way to test it without Selenium, Mechanize, or something similar. Sucks to be you.

So what is in that controller?

In Veure, you can "save" your character by paying money to clone it. Later, you if you die, your clone is restored. Creating a new clone is called "gestating" (and reverting to one "spawns" it) and some of the code looks like this:

my $guard = $self->result_source->schema->txn_scope_guard;
$character->dec_bank($price);
$character->update;

my $clone = $self->find_or_create(
    {
        character_id    => $character->character_id,
        station_area_id => $character->station_area->station_area_id,
    }
);
foreach my $attribute ($character->cloneable_attributes) {
    $clone->$attribute( $character->$attribute );
}
$clone->clone_date( DateTime->now );
$clone->update;
$guard->commit;

If you have that code in your controller class, you've now tightly coupled your controller to DBIx::Class and to test cloning, you have to go through your Web interface. Not good. Instead, push that logic into the model, perhaps in a resultset class, and make sure that your controller only calls that method. You can then test that logic without the Web interface and your tests will be faster and the error will show up quicker.

This is important. There are a number of times I need to check for the existence of NPCs in the code. Imagine if I had this in my controllers:

return $characters->search(
    { station_area_id => $station_area->station_area_id },
    { order_by => 'name' },
);

That doesn't seem too strange, but I don't want to duplicate that logic all over the place, particularly since it's wrong. It quickly became apparent that I only wanted to show characters that:

• Weren't onboard a ship (e.g., were on the station your character is at)
• And have been active in the last 24 hours
• ... unless they were an NPC, in which case, they're always visible

Had I duplicated character search throughout my controllers, sooner or later, that logic would get out of synch. At the current count, I have at least 15 places in the code where I need to know who is in a given station area. Fortunately, they all look like this:

my $characters = $c->model('DB::Character')->who($station_area);

(Yes, I'm sort of cheating by using resultsets for my model, but I'll fix it when it becomes unwieldy).

Now when I'm purchasing a new clone, my controller method used to look like this:

sub purchase : Local : Args(0) Does('NeedsLogin') {
    my ( $self, $c ) = @_;

    my $character = $c->current_character;
    my $price = $c->config->{clone_price_per_level} * $character->level;

    if ( $character->bank >= $price ) {
        $character->dec_bank($price);
        $character->update;
        $c->model('DB::Clone')->gestate($character);
        $c->add_message("A red pustule rapidly swells from the wall, your new clone floating inside it in the fetal position.");
    }
    else {
        $c->add_message("We're very sorry, but you don't have enough credits to gestate a new clone");
    }
}

That's too much code in the controller (and let's pretend I was using a transaction there, which I wasn't ... oops). But why did I have too much logic there? Because of those damned messages. I reasoned that the messages are used by Catalyst and the result classes shouldn't know what's being done with them. In fact, sometimes my code would jump through terrible contortions to return data and messages at the same time and I had to track the message passing carefully through the dbic layer. It was frustrating.

It was a code smell.

Messages are for one thing and one thing only: to tell you, the player, something relevant to your character. If there's no character, there's no message. Duh! That's when I did something just a wee bit unusual. In fact, it's a touch problematic, but it's tremendously made my code simpler: I'm using my Veure::Schema::Result::Character class to store those messages "out of band", if you will. My code in that class sort of looks like this:

sub add_message {
    my ( $self, $message ) = @_;
    return unless $message;
    push @{ $self->_message_store } => $message;
}

# otherwise, discard_changes() will wipe out my messages
sub discard_changes {
    my $self     = shift;
    my $messages = $self->messages;
    my $response = $self->next::method;
    $self->_message_store($messages);
    return $response;
}

sub clear_messages {
    my $self = shift;
    $self->_message_store( [] );
    return $self;
}

And now, whenever something happens that my character needs to know about, I can just call $character->add_message("You have refueled your ship for $cost credits"). I can save this data at the exact place the information is available and don't have to track down where it's generated at or pass it around anywhere. Inside of the Catalyst end action, I fetch those messages and then clear them. Later when I feel guilty about storing this extra information in a dbic result object, I've encapsulated the hack well enough that I can store those messages somewhere else ... if I ever need to.

And that purchase method above? It now looks like this:

sub purchase : Local : Args(0) Does('NeedsLogin') {
    my ( $self, $c ) = @_;
    $c->model('DB::Clone')->gestate( $c->current_character );
}

A very thin controller method indeed! In fact, quite a bit of code shrunk down just as dramatically as a result of this one change.

There's an interesting side benefit of removing all of the logic from the controllers and pushing them into the model layer: the controller is only a thin dispatching layer and the model is becoming a full-featured application in its own right. That means that later, if I want to build a phone app, it can talk directly to the application. If I wanted to create a command-line version of Veure, it can also talk directly to the application.

In other words, Veure can become something which is no longer a Web application, but a stand-alone MMORPG for which the Web site is merely one of many possible front-ends. That's a whopping huge amount of business value right there and it justifies thin controllers even if nothing else did.

There are still a few obstacles to getting to this point, but they're rapidly going away. Thin controllers are the way to go (just be careful and don't turn your model classes into god objects).