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.
Help the Symfony project!
As with any Open-Source project, contributing code or documentation is the most common way to help, but we also have a wide range of sponsoring opportunities.
Comments
Comments are closed.
To ensure that comments stay relevant, they are closed for old posts.
There are so many useful articles besides the documentation. How about indexing them and it would be eays to search for anyone~
Thanks fabien
little typo:
"use the new isMethod() object "
should be
"use the new isMethod() method"
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 ?
Thanks Fabien.
You have listed as a reusable method twice.
$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.
echo slot('product', $product->getTitle())
And you can't have a dynamic title in view.yml.
Thanks Fabien
@gaëtan: won't static and $this exclude each other? afaik you can't use $this within a static method.
Just a little thing: why don't use view.yml for CSS? (not for title)
Can we expect it to show up somewhere in the Documentation page for 1.1?
protected function getRequestProduct()
{
$this->forward404Unless($product = ProductPeer::retrieveByPk($this->getRequestParameter('id')));
return $product;
}
And it should be called like this:
$this->product = $this->getRequestProduct();
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.
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.