4
\$\begingroup\$

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($m) { return $m instanceof Newsletter; };

    $bag['sms_count']        = $outgoingMessages->filter($getSms)->count();
    $bag['newsletter_count'] = $outgoingMessages->filter($getNewsletters)->count();

    return $bag;
}
Improve this question
\$\endgroup\$
2
\$\begingroup\$

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!

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ To make it more clean, all data is used to render widgets on my dashboard. Would be better to create - say - statsWidgetAction, chartWidgetAction ans so on, letting the view render each widget as as single item? \$\endgroup\$ – gremo Jul 23 '12 at 7:55
  • \$\begingroup\$ @Gremo: If they are indeed different and are manipulated independently, then yes, I would believe separating them might be better. But at the same time I believe the redundancy introduced from doing so might be a bit much for such similar tasks. There's also the issue of unsynchonized data should your data be updated in real time. If each of the widgets are refreshed at the same time there might only be slight differences between them based on load time, or none at all. So there are a number of things to look at before deciding. In the end its up to you and your allowance for fault tolerance. \$\endgroup\$ – mseancole Jul 23 '12 at 14:51
-1
\$\begingroup\$

Without knowing anything about sympony's details:

  • I'd refactor most of the code into your view. I prefer to pass the required objects to the view and let the view pull the information it needs from the object(s)
  • Rename the variable bag
  • Move the subscription data into your user-model
  • Make the helpers available to your views (especially the charting helper)

This decouples the controller from the display logic. The controller just determines the user to display the dashboard for and the view determines which data to display and how to display it (e.g. you could have to different dashboards (views)).

public function dashboardAction()
{
    return array('user' => $this->getSecurityContext()->getToken()->getUser());
}

In your view (not knowing how views are done in symphony):

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

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

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

echo $charting->createMessagesLineChart($rawData, $start, $end);
echo $charting->createMessagesPieChart($rawData);

// Display the 5 newest customers
$customers = $loggedUser->getCustomers();

echo $customers->count();
echo $customers->slice(0, 5);

// Display the 5 latest Tags/keywords & total count
$metas       = $loggedUser->getMeta();
$getTags     = function($m) { return $m instanceof Tag; };
$getKeywords = function($m) { return $m instanceof Keyword; };

echo $metas->filter($getTags)->count();
echo $metas->filter($getKeywords)->count();
echo implode(",", $metas->slice(0, 5));

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

echo $outgoingMessages->filter($getSms)->count();
echo $outgoingMessages->filter($getNewsletters)->count();

Another view of the same dashboardAction just might decide to display only a welcome message, other information, or even be customizable by the user without changing the controller every time (possibly passing the superset of all variables required by any view to each view).

Going further you might want to move some get logic (which you're doing by filtering) into your model: $getTags = $loggedUser->getMeta()->getTags();

Improve this answer
\$\endgroup\$
5
  • 3
    \$\begingroup\$ This is not a very good example of a view. In fact, its a poor one. Views are supposed to be sparse in PHP code and be primarily HTML. You are combining all three here. Controllers are supposed to get data from the model and pass it to the view in an easy to use manner so that the view does not need to know anything about the controller or model. \$\endgroup\$ – mseancole Jul 19 '12 at 16:57
  • \$\begingroup\$ @showerhead: And it's not the controller's job to decide how the data is going to be displayed (e.g. as a chart). Sure we can argue if we pass an array to the view containing the content of the model or the model directly - resulting in the same result... (forgetting about what happens if we change the model). It's just accessing the data by either array-key or method-call. Ultimately some of the logic should be in the models (see last paragraph) or in view helpers. \$\endgroup\$ – Fge Jul 19 '12 at 19:37
  • \$\begingroup\$ No, its not the controller's job, its the view's. The view will request the chart from the controller, which will then request it from the model. The view should only have to say $chart to get a chart. The controller is responsible for calling the proper methods from the model to create that chart and then declare it as a variable before including the view. This way the only thing you have to change if the model changes is the controller. Which, if done correctly, may not need to be changed either. \$\endgroup\$ – mseancole Jul 19 '12 at 19:49
  • \$\begingroup\$ @showerhead: So far we're both talking about the same idea - mine as a 'lightweight' variant of viewmodels - your's as an array. Beside the chart you're almost returning the same data as I do - letting the view decide which data to display and how. If you want complete decoupling my solution would require complete viewmodels bloating the code for small applications. However using objects would enable lazy- loading/calculating stuff like graphs instead of pre-calculating the superset of required variables. (In my opinion creating charts is the view's task as they are about "displaying stuff".) \$\endgroup\$ – Fge Jul 19 '12 at 22:49
  • \$\begingroup\$ @showerhead: Guess we'll just come to the same result as this discussion here: stackoverflow.com/questions/1371119/… \$\endgroup\$ – Fge Jul 19 '12 at 22:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.