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

Some time ago, Vince, a seasoned PHP developer, asked me to have a look at his very first symfony project, a product store.

His customer was pretty happy as the website works well. Vince was also satisfied as he delivered the website on time. But he was not very confident about the quality of his code.

So, he sent me the code to have some feedback about its quality. Instead of replying by e-mail, I decided to invite him at Sensio Labs to work on a live refactoring session.

As the mistakes he did were quite common, I decided to tell you the story of this refactoring session in the hope you will learn as much as Vince did.

Today, we will describe some of the website features, and we will have a quick look at the code. We will start the refactoring process in the next part.

The project has been developed with symfony 1.1.

The schema

The database schema is composed of two tables, a product table, and a category table:

# config/schema.yml
propel:
  product:
    title:       { type: varchar(255), required: true }
    image:       varchar(255)
    description: longvarchar
    price:       { type: decimal, required: true }
    is_new:      boolean
    is_in_stock: boolean
    created_at:  timestamp
    category_id: ~
 
  category:
    name: varchar(255)
 

The homepage

The homepage of the website is composed of a list of all available products (is_in_stock set to true).

Product store homepage

All the features we will refactor are managed by the product module.

Here is the index action which manages the homepage:

// apps/frontend/modules/product/actions/actions.class.php
public function executeIndex()
{
  $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;
}
 

And the related indexSuccess.php template:

<h1>Our products</h1>
 
<?php foreach ($products as $product): ?>
  <div>
    <h2>
      <?php echo $product->getTitle() ?>
      <?php if ($product->getIsNew()): ?><span style="margin-left: 10px; color: #e55">NEW!</span><?php endif; ?>
    </h2>
    <div style="margin-bottom: 10px">
      <em>Category</em>: <?php echo $product->getCategory()->getName() ?> -
      <em>Price</em>: Only $<?php echo $product->getPrice() ?> - 
      <?php if (in_array($product->getId(), array_keys($sf_user->getAttribute('favorites', array())))): ?>
        <a href="<?php echo url_for('product/removeFromFavorites?id='.$product->getId()) ?>"><img src="/images/favorite.png" /></a>
      <?php else: ?>
        <small><?php echo link_to('add to my favorites', 'product/addToFavorites?id='.$product->getId()) ?></small>
      <?php endif; ?>
    </div>
    <div>
      <div style="float: left">
        <img width="100px" src="/images/products/<?php echo $product->getImage() ?>" />
      </div>
      <p>
        <?php echo $product->getDescription() ?>
        <?php if ($sf_user->isAuthenticated()): ?>
          <p style="text-align: right"><a href="<?php echo url_for('product/edit?id='.$product->getId()) ?>">Edit this product</a></p>
        <?php endif; ?>
      </p>
      <br style="clear: both" />
    </div>
    <div style="text-align: right">
      <?php echo link_to(image_tag('/images/add_to_cart.png'), 'product/buy?id='.$product->getId()) ?>
    </div>
    <hr />
  </div>
<?php endforeach; ?>
 

The favorites

As you might have seen on the homepage screenshot, the user can add a product to his favorites. The feature is quite simple: you click on the "add to my favorites" link to add a product to your favorites, and you click on the star image to remove it from your favorites.

The favorites are managed by the executeAddToFavorites() and executeRemoveFromFavorites() method from the product module:

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');
}
 

Product editing

When logged in, the administrator can also edit the product information.

Product editing

This page is managed by the edit action:

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())
    {
      // get the current product
      $product = $this->form->getObject();
      $image = $this->form->getValue('image');
      $currentImage = $product->getImage();
 
      if ($image)
      {
        // remove old image
        if (file_exists($file = sfConfig::get('sf_web_dir').'/images/products/'.$currentImage))
        {
          unlink($file);
        }
      }
 
      // update the product
      $product = $this->form->updateObject();
 
      if ($image)
      {
        // save new image
        $filename = sha1($image->getOriginalName()).$image->getExtension($image->getOriginalExtension());
        $image->save(sfConfig::get('sf_web_dir').'/images/products/'.$filename);
 
        $product->setImage($filename);
      }
      else
      {
        $product->setImage($currentImage);
      }
 
      // save the product
      $product->save();
 
      $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
      $this->redirect('product/index');
    }
  }
}
 

And the editSuccess.php template:

<h1>Edit "<?php echo $product->getTitle() ?>" <small>#<?php echo $product->getId() ?></small></h1>
 
<p><?php echo link_to('Back to the homepage', '@homepage') ?></p>
 
<form action="<?php echo url_for('product/edit?id='.$product->getId()) ?>" method="POST" enctype="multipart/form-data">
  <table cellspacing="0">
    <?php echo $form ?>
    <tr>
      <td colspan="2" style="text-align: right"><input type="submit" value="Save" /></td>
    </tr>
  </table>
</form>
 

That's all for today. We will start the refactoring process in the next part. In the meantime, take time to read the code and start thinking about what you would have refactored, how, and why.

Comments

Wow, this is going to be very useful !

Go on ! :)
A great idea - I'll give this a go. Will be interesting to see how many of my refactorings match up with yours!
Fabien, is this 1.0 or 1.1? The lack of $request in the actions made me think it was 1.0, but I've just spotted a 1.1 form object. Can you clarify for us? - thanks :o)
@halfer: it's a 1.1 project
@fabien: thanks.
Wonderful!! i realy learn more, thanks fabien.
Now i will prepare for symfony 1.2 ... :)
As you talk about sf1.1, Is it helpme to sf1.2 ?

Comments are closed.

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