• Fabien Potencier
• 10 months ago
• Call the expert
• 11

Today, we will start by refactoring some actions from the product module.

As symfony is modeled after the MVC design pattern, Vince was pretty confident that he did the separation of concerns pretty well.

The Controller role is to get data from the Model and pass them to the View.

Pretty simple, no?

A lot of people knows about the theory but it is hard to follow it by the letter in practice ; it is very easy to let some business logic slip into the controller. And Vince was no different.

Thin controller, fat model

The homepage of the website is composed of a list of all available products and the current code that manages this page is contained in the index action:

// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
  // get all available products
  $criteria = new Criteria();
  $criteria->add(ProductPeer::IS_IN_STOCK, true);
  $criteria->addAscendingOrderByColumn(ProductPeer::PRICE);
  $criteria->setLimit(10);
 
  $this->products = ProductPeer::doSelectJoinCategory($criteria);
 
  $this->getResponse()->setTitle('All products');
  $this->getResponse()->addStylesheet('homepage.css');
 
  return sfView::SUCCESS;
}
 

But The Criteria definition and the call to doSelectJoinCategory do not belong to the Controller, it belongs to the Model.

So, Vince decided to move this code to the ProductPeer class by creating a getAvailableProducts() method:

// lib/model/ProductPeer.php
static public function getAvailableProducts()
{
  $criteria = new Criteria();
  $criteria->add(self::IS_IN_STOCK, true);
  $criteria->addAscendingOrderByColumn(self::PRICE);
  $criteria->setLimit(10);
 
  return self::doSelectJoinCategory($criteria);
}
 

And the controller can now use this new method directly:

// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
  $this->products = ProductPeer::getAvailableProducts();
 
  $this->getResponse()->setTitle('All products');
  $this->getResponse()->addStylesheet('homepage.css');
 
  return sfView::SUCCESS;
}
 

This refactoring has several benefits over the previous code:

• The logic to get the available products is now in the Model, where it belongs
• The code in the controller is much more readable
• The code is so readable that we do not need to document it anymore
• The getAvailableProducts() method is re-usable (in another action, or a in task, ...)
• The model code is now unit testable
• Two developers can work together:
  • one is responsible to code the Model and to create a public API out of it
  • the other one uses the Model public API to code the Controller. And she does not care about how the products are retrieved (database, XML, Web Services...)

But what if you need the list of all available products in another action, or if you only need to get the five first ones? You will want to reuse the same method but as it is now, it is not very flexible as the limit (10) is hardcoded.

It is fine to keep it that way for now, but as soon as Vince will need to get the same type of results but with slightly different conditions, he will need refactor the new method to add some arguments:

// lib/model/ProductPeer.php
static public function getAvailableProducts($max = 10)
{
  $criteria = new Criteria();
  $criteria->add(self::IS_IN_STOCK, true);
  $criteria->addAscendingOrderByColumn(self::PRICE);
  $criteria->setLimit($max);
 
  return self::doSelectJoinCategory($criteria);
}
 

You get the idea. The goal is to be able to reuse the same code in different contexts. The first step is to refactor the code in the right layer and then add some way to customize the results by adding some arguments.

This one was easy. Let's refactor another part of the code.

Thin controller, fat model, revisited

Time to have a look at the favorite management code.

The code to add and remove products from the favorites is to be found in the product module:

// apps/frontend/modules/product/actions.class.php
public function executeAddToFavorites()
{
  $product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  $this->forward404Unless($product);
 
  $favorites = $this->getUser()->getAttribute('favorites');
  $favorites[$product->getId()] = true;
 
  $this->getUser()->setAttribute('favorites', $favorites);
 
  $this->redirect('product/index');
}
 
public function executeRemoveFromFavorites()
{
  $product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  $this->forward404Unless($product);
 
  $favorites = $this->getUser()->getAttribute('favorites');
  unset($favorites[$product->getId()]);
 
  $this->getUser()->setAttribute('favorites', $favorites);
 
  $this->redirect('product/index');
}
 

As you can see for yourself, the favorites are stored in the user session. But again, too much logic is embedded in this code. The controller does not need to know where and how the favorites are stored.

So, Vince decided to move this code to the myUser class by creating methods to encapsulate the logic:

// apps/frontend/lib/myUser.class.php
public function addProductToFavorites(Product $product)
{
  $favorites = $this->getAttribute('favorites');
  $favorites[$product->getId()] = true;
 
  $this->setAttribute('favorites', $favorites);
}
 
public function removeProductFromFavorites(Product $product)
{
  $favorites = $this->getAttribute('favorites');
  unset($favorites[$product->getId()]);
 
  $this->setAttribute('favorites', $favorites);
}
 

And the refactored actions in the product module class now uses this new user API:

// apps/frontend/modules/product/actions.class.php
public function executeAddToFavorites()
{
  $product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  $this->forward404Unless($product);
 
  $this->getUser()->addProductToFavorites($product);
 
  $this->redirect('product/index');
}
 
public function executeRemoveFromFavorites()
{
  $product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  $this->forward404Unless($product);
 
  $this->getUser()->removeProductFromFavorites($product);
 
  $this->redirect('product/index');
}
 

This refactoring has the same benefits as the one we did previously, but with an added bonus: if we decide to change the attribute name, or if we want to store the favorites in the database, the controller code won't change.

And the same goes for the template. Here is the snippet of code that deals with the favorites:

// apps/frontend/modules/product/templates/indexSuccess.php
<?php if (in_array($product->getId(), array_keys($sf_user->getAttribute('favorites', array())))): ?>
  <?php echo link_to(image_tag('/images/favorite.png'), 'product/removeFromFavorites?id='.$product->getId()) ?>
<?php else: ?>
  <?php echo link_to('add to my favorites', 'product/addToFavorites?id='.$product->getId()) ?>
<?php endif; ?>
 

The first line tests if the product is in the current user favorites to display the "add" link or the "remove" link.

Here again, we need to move this code to the user class by creating another method:

// apps/frontend/lib/myUser.class.php
public function hasInFavorites(Product $product)
{
  return in_array($product->getId(), array_keys($this->getAttribute('favorites', array())));
}
 

And here is the cleaned up template snippet:

// apps/frontend/modules/product/templates/indexSuccess.php
<?php if ($sf_user->hasInFavorites($product)): ?>
  <?php echo link_to(image_tag('/images/favorite.png'), 'product/removeFromFavorites?id='.$product->getId()) ?>
<?php else: ?>
  <?php echo link_to('add to my favorites', 'product/addToFavorites?id='.$product->getId()) ?>
<?php endif; ?>
 

And we did it again. The code is more readable thanks to the refactoring and the method name we chose. It is very important to think about method names. They convey a lot of information and most of the time, they can replace the documentation. You don't need to explain what $sf_user->hasInFavorites($product) does.

After this refactoring, all the code that deals with the favorites is now in the same class. And that's yet another benefit. Instead of having code that manipulates the favorites in a lot of different places, everything is now centralized in one class. You can even refactor it further by creating a dedicated ProductFavorite class if the need arises.

That's all for today. Next time, Vince and I will refactor the action that allows to modify the product and the associated image. In the meantime, you can apply the rules we have learned today to your own symfony projects.

Comments

• 

  #1 Alex said about 2 hours later

  Nice article Fabien! Great tips all PHP developers should consider - not just symfony developers.

• 

  #2 Piwaï said about 2 hours later

  "A lot of people knows about the theory but it is hard to follow it by the letter in practice" => OMG I did wrong too.. now I know how to refactor my code ;) .

• 

  #3 Piwaï said about 3 hours later

  2 questions linked to that kind of refactoring :

  1) Should the creation of a new product be also written in ProductPeer ? I guess the answer is yes.

  2) What about classes brought by plugins ? For instance, sfGuardUser : where should we add the specific code, since I guess it would not be nice to hard code it into the sfGuardPlugin directory..

• 

  #4 John Wards said about 8 hours later

  @Piwaï

  1) Yes.
  2) Create your own classes that extend the modules provided by plugins.

• 

  #5 Hugo said about 8 hours later

  Very nice and well written article ! I hope to read the next parts of this tutorial soon :)

• 

  #6 Ice_j7 said about 10 hours later

  I did the same with my model, but not with myUser. Great article indeed.

• 

  #7 Chris said about 16 hours later

  Where are the unit tests?

• 

  #8 adrive said 1 day later

  John Wards:
  But what if I have two plugins CommentPlugin and PollPlugin and I am using sfGuardPlugin.

  I need to have two methods: I need to test, whether user already read some comments and I need to test, whether user has already voted in the poll.

  I can't extend sfGuardSecurityUser because I do not know, in which order I will use plugins and which one will extend from another.

  That's because I create something like "ProducFavorite" - "PollVoter", "CommentReader" which manipulates with sfContext::getUser()...

• 

  #9 Luã said 1 day later

  Wow, i was doing it wrong too.
  Thanks for this tips, it's a very helpfull tuto.

• 

  #10 Jerome Texier said 3 days later

  In order to refactor product action code, and to respect DRY, what about in this simple case, not moving
  $product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  $this->forward404Unless($product);
  to a preExecute function
  and
  $this->redirect('product/index');
  to a postExecute one ?
  So executeAddToFavorites() and executeRemoveFromFavorites() are each downsized to a single line.

• 

  #11 Alexey Kupershtokh said 12 days later

  public function hasInFavorites(Product $product)
  { return in_array($product->getId(), array_keys($this->getAttribute('favorites', array())));
  }

  I would better write:

  public function hasInFavorites(Product $product)
  { $favorites = $this->getAttribute('favorites', array());
  return isset($favorites[$product->getId()]);
  }

  This tiny optimization allows this code perform in O(1) instead of O(n) (where n is number of current favorites).