patternphpMinor
PHP MVC controller code needs diet?
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($
```
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.
Hope this helps!
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.