HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP MVC controller code needs diet?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mvcneedsphpdietcontrollercode

Problem

This is the dashboard controller code in PHP Symfony 2. It collects some aggregate data and points for charts, but i don't like it very much. Do you think that this code belongs to what a controller should do in a MVC patter? How can i refactor it?

```
public function dashboardAction()
{
$bag = array();

// Helpers from service container
$messaging = $this->get('messaging.helper');
$charting = $this->get('charting.helper');

// Current logged user, subscription and message inhibitor
$loggedUser = $this->getSecurityContext()->getToken()->getUser();
$subscription = $messaging->createSubscriptionFromUser($loggedUser);
$usage = $messaging->createUsageFromSubscription($subscription);

$bag['subscription'] = $subscription;
$bag['usage'] = $usage;
$bag['inhibitor'] = $messaging->createInhibitor($usage, $subscription);

// Get charts from usage raw data
$rawData = $usage->getRawData();
$start = $usage->getStartDate();
$end = $usage->getEndDate();

$bag['line_chart'] = $charting->createMessagesLineChart($rawData, $start, $end);
$bag['pie_chart'] = $charting->createMessagesPieChart($rawData);

// Customers and latest 5 added
$customers = $loggedUser->getCustomers();

$bag['customers_count'] = $customers->count();
$bag['latest_customers'] = $customers->slice(0, 5);

// Tags/keywords counts and latest 5 meta
$metas = $loggedUser->getMeta();
$getTags = function($m) { return $m instanceof Tag; };
$getKeywords = function($m) { return $m instanceof Keyword; };

$bag['tags_count'] = $metas->filter($getTags)->count();
$bag['keywords_count'] = $metas->filter($getKeywords)->count();
$bag['latest_meta'] = $metas->slice(0, 5);

// SMS and newsletter counts
$outgoingMessages = $loggedUser->getOutgoingMessages();
$getSms = function($m) { return $m instanceof SmallTextMessage; };
$getNewsletters = function($

Solution

A controller? No. A model? Sure. Not sure if Symfony defines a controller differently, but this is not what I think of as a controller.

As for refactoring, the easiest way would be to break it up into smaller methods. For instance, right off the bat I see at least 6 different potential methods here. You could create a bunch of smaller methods that returns a part of the bag (messaging, charting, etc...) and then merge them all together. In fact, that's the only real improvement I can see. I feel bad leaving such a short answer, so here's an example of a couple of those smaller methods, which is actually about half that code.

private function getUsageCharts ( $usage ) {
    $rawData = $usage->getRawData();
    $start   = $usage->getStartDate();
    $end     = $usage->getEndDate();

    $line_chart = $charting->createMessagesLineChart($rawData, $start, $end);
    $pie_chart  = $charting->createMessagesPieChart($rawData);

    return compact( 'usage', 'line_chart', 'pie_chart' );
}

private function getMessagingFields( $loggedUser ) {
    $messaging = $this->get('messaging.helper');

    $subscription = $messaging->createSubscriptionFromUser($loggedUser);
    $usage = $messaging->createUsageFromSubscription($subscription);
    $inhibitor = $messaging->createInhibitor($usage, $subscription);

    extract( $this->getUsageCharts( $usage ) );//$line_chart, $pie_chart
    return compact( 'subscription', 'usage', 'inhibitor', 'line_chard', 'pie_chart' );
}


Hope this helps!

Code Snippets

private function getUsageCharts ( $usage ) {
    $rawData = $usage->getRawData();
    $start   = $usage->getStartDate();
    $end     = $usage->getEndDate();

    $line_chart = $charting->createMessagesLineChart($rawData, $start, $end);
    $pie_chart  = $charting->createMessagesPieChart($rawData);

    return compact( 'usage', 'line_chart', 'pie_chart' );
}

private function getMessagingFields( $loggedUser ) {
    $messaging = $this->get('messaging.helper');

    $subscription = $messaging->createSubscriptionFromUser($loggedUser);
    $usage = $messaging->createUsageFromSubscription($subscription);
    $inhibitor = $messaging->createInhibitor($usage, $subscription);

    extract( $this->getUsageCharts( $usage ) );//$line_chart, $pie_chart
    return compact( 'subscription', 'usage', 'inhibitor', 'line_chard', 'pie_chart' );
}

Context

StackExchange Code Review Q#13742, answer score: 2

Revisions (0)

No revisions yet.