Does my controller code look good?

Question

I read many times about controller code mustn't be too complicated and so on. I was developing new features in my simple project. I added a function, which allow users to get access to news only in one specified category. Now, if a user writes some of this URL:

/news/common
/news/sport
/news/finance

only news from specified category would be shown.

I was thinking about how to do this through another actions, but realized that I can do it in index action. I need just to check if user entered category, but not id (id can contain only digits), which I've done.

Controller:

public function indexAction() {
 $objectManager = $this->getServiceLocator()->get('Doctrine\ORM\EntityManager');
 $options = array();
 $categoryUrl = (string)$this->params('category');
 if($categoryUrl) { // add category to the 'where'
 $category = $objectManager
 ->getRepository('\News\Entity\Category')
 ->findOneByUrl($categoryUrl);
 if(!$category) {
 return $this->redirect()->toRoute('news');
 }
 $options['category'] = $category->getId();
 $categoryName = $category->getName();
 }
 $news = $objectManager
 ->getRepository('\News\Entity\Item')
 ->findBy($options, array('created'=>'DESC'));
 $items = array();
 foreach ($news as $item) {
 $buffer = $item->getArrayCopy();
 $buffer['category'] = $item->getCategory()->getName();
 $buffer['user'] = $item->getUser()->getDisplayName();
 $items[] = $buffer;
 }
 $view = new ViewModel(array(
 'news' => $items,
 'categoryName' => $categoryName,
 ));
 return $view;
}

What I am doing here?

• receive category from URL
  • if category specified I add clause to the $options array and set $categoryName as category name
  • if category is not specified I don't do anything with $options (so it will be blank after this part) and don't set flag (so it is not set, undefined)
• get news items (function pass $options array)
• return $categoryName and news array to the view

View:

<? 
 if($categoryName) {
 $title = $categoryName;
 } else {
 $title = "News list";
 }
 $this->headTitle($title); 
?>
// html code, some conditions etc

There is an if condition. If $categoryName is specified, $title will have the same contents as $categoryName. If $categoryName is not specified, $title will be just News list.

Questions

• Is this the correct approach at all? Should I create new actions and handle this case in it?
• Is it correct to set flags, as I did, send to the view, handle it etc?
• Is my controller "fat" now?
• How can I improve this code?

In addition, you can find the full code of files on GitHub:

• NewsController.php (controller)
• index.phtml (view)

Note: Some words in files are in Russian.

Answer 1

On the surface the code in your question seems fine, but I can tell you from experience that once this door is cracked just a bit, it will continue to creak open wider over time.

• "I'm just checking if we have an ID or category name."
• "This just adds a few meta tags."
• "It's already 200 lines; 50 more won't matter."

The linked controller, however, is doing way too much work. It should be passing data off to a model class (not the entity manager directly), placing whatever the view needs into the ViewModel, and that's it. Controllers are glue code. As you have it, you'll need to copy all of this code and modify it slightly to expose the CRUD interface in another form.

For the code you posted, I would prefer to separate the actions so each handles one specific use case: all items, items matching a category, and one item (not in the code but you mentioned it). Create a regex route for the last two. The beauty of this is that you don't need to do all the conditional checks--the dispatcher does it for you.

// see miscellaneous tips below
public function init() {
 $this->objectManager = $this->getServiceLocator()
 ->get('Doctrine\ORM\EntityManager');
}
public function allAction() {
 return new ViewModel(array(
 'news' => $this->loadItems(),
 'categoryName' => null,
 ));
}
public function categoryAction() {
 $category = $this->objectManager
 ->getRepository('\News\Entity\Category')
 ->findOneByUrl((string) $this->params('category'));
 if (!$category) {
 return $this->redirect()->toRoute('news');
 }
 return new ViewModel(array(
 'news' => $this->loadItems(array('category' => $category->getId())),
 'categoryName' => $categoryName,
 ));
}
private function loadItems($options = array()) {
 $news = $this->objectManager
 ->getRepository('\News\Entity\Item')
 ->findBy($options, array('created' => 'DESC'));
 $items = array();
 foreach ($news as $item) {
 $buffer = $item->getArrayCopy();
 $buffer['category'] = $item->getCategory()->getName();
 $buffer['user'] = $item->getUser()->getDisplayName();
 $items[] = $buffer;
 }
 return $items;
}

This is about the same length of the original, but it's far less complicated. Each action is easy to follow and clearly lays out what it requires.

Miscellaneous

And here are a few tips after looking at your linked controller and view code:

• You are accessing the entity manager in every action (sometimes pulling it from the registry twice in the same method). Do this once by storing it in an instance property in init.

• The index and list actions are nearly identical. Refactor these to extract the common code into a new private method. It looks like this applies to some of the other actions, e.g., converting a news item into an array for the view with its associated category and user names.

• You can simplify the title-setting with the Elvis operator: $title = categoryName ?: "Список новостей".

• Every page should have an H1--even the "all news" index page.

• An non-empty array is truthy in PHP. if(count($this->news) != 0): can be shortened to if($this->news):. If you want to be explicit, at least use if(!empty($this->news)):.

• If $this->news will be an empty array instead of null or false when there are no items, you don't even need the if since looping over an empty array is a no-op.

• You don't need the ; in <?=...?> since it's an expression instead of a statement. You also don't need it for one-line statements in <?php ... ?>, but we still use it to avoid bugs when someone adds a line.

• Sharikov Vladislav: Oh! This is so full and good answer. Thank you very much. Your code is looking much more "readable" (not sure that translation is correct or there are such word in english :'( ) and lesser complicated as mine. And thank you for tips. It is good ideas. Well, I am just learning :) Thank you.