Call the expert: A refactoring story (Part 5/5)

In the last part, we have moved some code to ProductForm. Today, we will enhance the actions even more by moving code to the View and by using some nice shortcuts provided by symfony for common situations.

In part 3, we have refactored the index action which now looks like this:

// 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;
}
 

In symfony, the default view is sfView::SUCCESS, so we can omit it from the action:

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

In this action, we set the page title and we add a specific stylesheet. Even if this code works as expected, it is not the right way to do this. This code really belongs to the view. So let's move this code to the indexSuccess.php template, so that our action looks like this:

// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
  $this->products = ProductPeer::getAvailableProducts();
}
 

To add a stylesheet in a template, we can use the use_stylesheet() helper:

<!-- apps/frontend/modules/product/templates/indexSuccess.php -->
<?php use_stylesheet('homepage.css');
 
<h1>Our products</h1>
 
<?php foreach ($products as $product): ?>
<!-- ... -->
 

To move the title from the action to the template, it's a bit more involving. We need to create a slot in the layout that we will populate in the template:

<!-- apps/frontend/modules/templates/layout.php -->
<html>
  <head>
    <title><?php include_slot('title') ?></title>
  </head>
  <body>
    <!-- ... -->
  </body>
</html>
 
<!-- apps/frontend/modules/product/templates/indexSuccess.php -->
<?php use_stylesheet('homepage.css');
<?php slot('title', 'All products') ?>
 
<h1>Our products</h1>
 
<?php foreach ($products as $product): ?>
<!-- ... -->
 

Slots are a nice way to add some dynamics parts in the layout that can be overridden by each template.

Let's move on to the edit action. Here is the current code:

public function executeEdit()
{
  $this->product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  if (is_null($this->product))
  {
    $this->forward404();
  }
 
  $this->form = new ProductForm($this->product);
 
  if (sfRequest::POST == $this->getRequest()->getMethod())
  {
    $this->form->bind($this->getRequestParameter('product'), $this->getRequest()->getFiles('product'));
    if ($this->form->isValid())
    {
      $product = $this->form->save();
 
      $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
      $this->redirect('product/index');
    }
  }
}
 

As it is very common to forward to a 404 page when a condition is met, symfony has some nice shortcut methods: forward404If() and forward404Unless():

public function executeEdit()
{
  $this->forward404Unless($this->product = ProductPeer::retrieveByPk($this->getRequestParameter('id')));
 
  // ...
}
 

As of symfony 1.1, symfony passes the request object as the first argument to the execute* methods. So, instead of using the getRequestParameter() proxy method, we can just use the getParameter() method from the request object. We can also use the new isMethod() method to simplify the POST check.

Here is the final executeEdit() method after Vince made all the changes:

public function executeEdit($request)
{
  $this->forward404Unless($this->product = ProductPeer::retrieveByPk($request->getParameter('id')));
 
  $this->form = new ProductForm($this->product);
  if ($request->isMethod('POST'))
  {
    $this->form->bind($request->getParameter('product'), $request->getFiles('product'));
    if ($this->form->isValid())
    {
      $product = $this->form->save();
 
      $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
      $this->redirect('product/index');
    }
  }
}
 

The executeEdit() code is now so common, that you can use a nice shortcut for the form processing. The bindAndSave() method is a shortcut that calls the bind() method and then save() the object if the form is valid:

public function executeEdit($request)
{
  $this->forward404Unless($this->product = ProductPeer::retrieveByPk($request->getParameter('id')));
 
  $this->form = new ProductForm($this->product);
  if ($request->isMethod('POST') && $this->form->bindAndSave($request->getParameter('product'), $request->getFiles('product')))
  {
    $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
    $this->redirect('product/index');
  }
}
 

The Controller purpose is now expressed very nicely. Here is the final code for the product module:

// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
  $this->products = ProductPeer::getAvailableProducts();
}
 
public function executeEdit($request)
{
  $this->forward404Unless($this->product = ProductPeer::retrieveByPk($request->getParameter('id')));
 
  $this->form = new ProductForm($this->product);
  if ($request->isMethod('POST') && $this->form->bindAndSave($request->getParameter('product'), $request->getFiles('product')))
  {
    $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
    $this->redirect('product/index');
  }
}
 
public function executeAddToFavorites($request)
{
  $this->forward404Unless($product = ProductPeer::retrieveByPk($request->getParameter('id')));
 
  $this->getUser()->addProductToFavorites($product);
 
  $this->redirect('product/index');
}
 
public function executeRemoveFromFavorites($request)
{
  $this->forward404Unless($product = ProductPeer::retrieveByPk($request->getParameter('id')));
 
  $this->getUser()->removeProductFromFavorites($product);
 
  $this->redirect('product/index');
}
 

The new productActions class has now 31 lines of code instead of 96. We have cut the code by more than two-thirds!

Here is a LOC table for our project:

Layer Before After
Model
Product 0 9
User 0 19
View
Index 35 37
Controller
Actions 96 31
Forms 7 24
Total 138 120

So, after the refactoring, we have a bit less of code and especially a better separation of concerns. We also have created re-usable objects and methods:

  • ProductForm
  • ProductPeer::getAvailableProducts()
  • myUser::addProductToFavorites()
  • myUser::removeProductFromFavorites()
  • myUser::hasInFavorites()

And reusable objects also means that they are self-contained. So, you can even package your model classes and forms as a plugin now.

As an added bonus, writing unit tests for our code is now also much easier.

The main goals of every refactoring process are always the same:

  • make the code more maintainable
  • allow the code to evolve easily
  • reduce the amount of code needed to write new features by reusing existing code

Even if Vince was quite tired after this refactoring session, he was pretty happy and started to refactor other parts of his website right away. I hope you have also learned new tricks that you will be able to use for your next symfony project.

Comments

Excellent work Fabien!
There are so many useful articles besides the documentation. How about indexing them and it would be eays to search for anyone~
Of course we learnt new great tricks.
Thanks fabien
very nice tutorial!!

little typo:
"use the new isMethod() object "
should be
"use the new isMethod() method"
Great tutorial, learning is definitively easier when you work an a concrete case. I think that everybody would enjoy to have other articles like this serie :)

I still have a question : why didn't you use a view.yml at the module level to set the title and the css file ?
For readability, and erros saving, isn't configuration file editing better than code writing ?
Very interesting articles, hope to more things like this hit the documentation :-).

Thanks Fabien.
@Markus.Staab: fixed
myUser::addProductToFavorites()

You have listed as a reusable method twice.
This line is repeated 3 times in the controller:

$this->forward404Unless($product = ProductPeer::retrieveByPk($request->getParameter('id')));

I personnaly would have put it in a static protected method of the controller, say:

static protected function getRequestProduct()
{
$this->forward404Unless($product = ProductPeer::retrieveByPk($this->getRequestParameter('id')));

return $product;
}

and then eg. in executeEdit:

$this->product = self::getRequestProduct();

This would allow to have a single point to define the way a product should be retrieved from the request informations.
@Ian Dominey: fixed, thanks
@Eric: I haven't used the view.yml configuration for the title because sometimes, it can be dynamic:

echo slot('product', $product->getTitle())

And you can't have a dynamic title in view.yml.

@Gaëtan: That's not going to work, the method must not be static (as you can't call $this->... in a static method).
Very useful. Hope to see more of this kind of refactoring tutorial in the future !

Thanks Fabien
@fabien: in executeEdit () you write $this->product = ... and later on $product->getTitle(). anyway, nice tutorial :).

@gaëtan: won't static and $this exclude each other? afaik you can't use $this within a static method.
Nice! Very Nice! thank you so much, this kind of "best practice" articles were missing, hope to see more soon! ;)
Thanks for these tutorials :) Hope to see more articles like these soon :p
I think the php code that includes the title slot should be nested in title tags, n'est-ce pas??
@Jonathan: fixed, thanks.
Should set_slot() above be replaced with slot(), at least on sf 1.1?
really nice tutorial!! Hope to see more of this kind of tutorial!
Very interesting!

Just a little thing: why don't use view.yml for CSS? (not for title)
@Nicolas: This is really just a matter of taste. I like to add my stylesheets in the templates/layouts directly.
This a great series, looking forward to more like it.

Can we expect it to show up somewhere in the Documentation page for 1.1?
@oktay> oops, you're right. The method shouldn't be static, just like this:

protected function getRequestProduct()
{
$this->forward404Unless($product = ProductPeer::retrieveByPk($this->getRequestParameter('id')));

return $product;
}

And it should be called like this:

$this->product = $this->getRequestProduct();
Great lesson of refactoring! thx
I like the idea of doing the title your way, but by doing the title this way how is the meta title handled?
@Stephen Ostrow: A slot is reusable several times:

Nice work Fabien.
You could have conclude that even Vince was tired, he ran his functional test (only one command line). All the tests passed so he got confident to haven't broken the application.
Which is a quite important goal in our job.
Thanks Fabien. Great learning.
I wanted to do some refactoring in my project (still symfony 1.0). I have "products" that spread across several tables (optional configurable addons to products). I need to get the logic out of the controller and I'm thinking where to put the logic into. Model? Which model table, as I'm using several. In the model of the main product table? Or in a separate library class?

I'm not yet fluent with best practice of this separation. Thanks for a hint.

Comments are closed.

To ensure that comments stay relevant, they are closed for old posts.