Day 04: refactoring

Previously on symfony

During day three, all the layers of a MVC architecture were shown and modified to have the list of questions properly displayed on the homepage. The application is getting nicer but still lacks content.

The objectives for the fourth day are to show the list of answers to a question, to give a nice URL to the question detail page, to add a custom class, and to move some chunk of codes to a better place. This should help you understand the concepts of template, model, routing policy, and refactoring. You may think that it is too early to rewrite code that is only a few days old, but we'll see how you feel about it at the end of this tutorial.

To read this tutorial, you should be familiar with the concepts of the MVC implementation in symfony. It would also help if you had an idea about what agile development is.

Show the answers to a question

First, let's continue the adaptation of the templates generated by the Question CRUD during day two

The question/show action is dedicated to display the details of a question, provided that you pass it an id. To test it, just call (you will have to change 2 to the real id the question has in your table):

http://askeet/frontend_dev.php/question/show/id/2

question detail

You probably already saw the show page if you played with the application before. This is where we are going to add the answers to a question.

A quick look at the action

First, let's have a look at the show action, located in the askeet/apps/frontend/modules/question/actions/actions.class.php file:

public function executeShow()
 {
   $this->question = QuestionPeer::retrieveByPk($this->getRequestParameter('id'));
   $this->forward404Unless($this->question);
 }

If you are familiar with Propel, you recognize here a simple request to the Question table. It is aimed to get the unique record having the value of the id parameter of the request as a primary key. In the example given in the URL above, the id parameter has a value of 1, so the ->retrieveByPk() method of the QuestionPeer class will return the object of class Question with 1 as a primary key. If you are not familiar with Propel, come back after you've read some documentation on their website.

The result of this request is passed to the showSuccess.php template through the $question variable.

The ->getRequestParameter('id') method of the sfAction object gets... the request parameter called id, whether it is passed in a GET or in a POST mode. For instance, if you require:

http://askeet/frontend_dev.php/question/show/id/1/myparam/myvalue

...then the show action will be able to retrieve myvalue by requesting $this->getRequestParameter('myparam').

Modify the showSuccess.php template

The generated showSuccess.php template is not exactly what we need, so we will completely rewrite it. Open the frontend/modules/question/templates/showSuccess.php file and replace its content by:

<?php use_helper('Date') ?>
 
<div class="interested_block">
  <div class="interested_mark" id="mark_<?php echo $question->getId() ?>">
    <?php echo count($question->getInterests()) ?>
  </div>
</div>
 
<h2><?php echo $question->getTitle() ?></h2>
 
<div class="question_body">
  <?php echo $question->getBody() ?>
</div>
 
<div id="answers">
<?php foreach ($question->getAnswers() as $answer): ?>
  <div class="answer">
    posted by <?php echo $answer->getUser()->getFirstName().' '.$answer->getUser()->getLastName() ?> 
    on <?php echo format_date($answer->getCreatedAt(), 'p') ?>
    <div>
      <?php echo $answer->getBody() ?>
    </div>
  </div>
<?php endforeach; ?>
</div>

You recognize here the interested_block div that was already added to the listSuccess.php template yesterday. It just displays the number of interested users for a given question. After that, the markup also looks very much like the one of the list, except that there is no link_to on the title. It is just a rewriting of the initial code to display only the necessary information about a question.

The new part is the answers div. It displays all the answers to the question (using the simple $question->getAnswers() Propel method), and for each of them, shows the total relevancy, the name of the author, and the creation date in addition to the body.

The format_date() is another example of template helpers for which an initial declaration is required. You can find more about this helper's syntax and other helpers in the internationalization helpers chapter of the symfony book (these helpers speed up the tedious task of displaying dates in a good looking format).

Add some new test data

It is time to add some data for the answer and relevancy tables at the end of the data/fixtures/test_data.yml (feel free to add your own):

Answer:
  a1_q1:
    question_id: q1
    user_id:     francois
    body:        |
      You can try to read her poetry. Chicks love that kind of things.

  a2_q1:
    question_id: q1
    user_id:     fabien
    body:        |
      Don't bring her to a donuts shop. Ever. Girls don't like to be
      seen eating with their fingers - although it's nice. 

  a3_q2:
    question_id: q2
    user_id:     fabien
    body:        |
      The answer is in the question: buy her a step, so she can 
      get some exercise and be grateful for the weight she will
      lose.

  a4_q3:
    question_id: q3
    user_id:     fabien
    body:        |
      Build it with symfony - and people will love it.

Reload your data with:

$ php batch/load_data.php

Navigate to the action showing the first question to check if the modifications were successful:

http://askeet/frontend_dev.php/question/show/id/XX

The question is now displayed in a fancier way, followed by the answers to it. That's better, isn't it?

Modify the model, part I

It is almost certain that the full name of an author will be needed somewhere else in the application. You can also consider that the full name is an attribute of the User object. This means that there should be a method in the User model allowing to retrieve the full name, instead of reconstructing it in an action. Let's write it. Open the askeet/lib/model/User.php and add in the following method:

public function __toString()
{
  return $this->getFirstName().' '.$this->getLastName();
}

Why is this method named __toString() instead of getFullName() or something similar? Because the __toString() method is the default method used by PHP5 for object representation as string. This means that you can replace the

posted by <?php echo $answer->getUser()->getFirstName().' '.$answer->getUser()->getLastName() ?> 

line of the askeet/apps/frontend/modules/question/templates/showSuccess.php template by a simpler

posted by <?php echo $answer->getUser() ?> 

to achieve the same result. Neat, isn't it ?

Don't repeat yourself

One of the good principles of agile development is to avoid duplicating code. It says "Don't Repeat Yourself" (D.R.Y.). This is because duplicated code is twice as long to review, modify, test and validate than a unique encapsulated chunk of code. It also makes application maintenance much more complex. And if you paid attention to the last part of today's tutorial, you probably noticed some duplicated code between the listSuccess.php template written yesterday and the showSuccess.php template:

<div class="interested_block">
  <div class="interested_mark" id="mark_<?php echo $question->getId() ?>">
    <?php echo count($question->getInterests()) ?>
  </div>
</div>

So our first session of refactoring will remove this chunk of code from the two templates and put it in a fragment, or reusable chunk of code. Create an _interested_user.php file in the askeet/apps/frontend/modules/question/templates/ directory with the following code:

<div class="interested_mark" id="mark_<?php echo $question->getId() ?>">
  <?php echo count($question->getInterests()) ?>
</div>

Then, replace the original code in both templates (listSuccess.php and showSuccess.php) with:

<div class="interested_block">
  <?php include_partial('interested_user', array('question' => $question)) ?>
</div>

A fragment doesn't have native access to any of the current objects. The fragment uses a $question variable, so it has to be defined in the include_partial call. The additional _ in front of the fragment file name helps to easily distinguish fragments from actual templates in the templates/ directories. If you want to learn more about fragments, read the view chapter of the symfony book.

Modify the model, part II

The $question->getInterests() call of the new fragment does a request to the database and returns an array of objects of class Interest. This is a heavy request for just a number of interested persons, and it might load the database too much. Remember that this call is also done in the listSuccess.php template, but this time in a loop, for each question of the list. It would be a good idea to optimize it.

One good solution is to add a column to the Question table called interested_users, and to update this column each time an interest about the question is created.

Add a field to the User object model

Go without fear and modify the askeet/config/schema.xml data model by adding to the ask_question table:

<column name="interested_users" type="integer" default="0" />

Then rebuild the model:

$ symfony propel-build-model

That's right, we are already rebuilding the model without worrying about existing extensions to it! This is because the extension to the User class was made in the askeet/lib/model/User.php, which inherits from the Propel generated askeet/lib/model/om/BaseUser.php class. That's why you should never edit the code of the askeet/lib/model/om/ directory: it is overridden each time a build-model is called. Symfony helps to ease the normal life cycle of model changes in the early stages of any web project.

You also need to update the actual database. To avoid writing some SQL statement, you should rebuild your SQL schema and reload your test data:

$ symfony propel-build-sql
$ mysql -u youruser -p askeet < data/sql/lib.model.schema.sql
$ php batch/load_data.php

$ mysql -u youruser -p askeet -e "alter table ask_question add interested_users int default '0'"

Modify the save() method of the Interest object

Updating the value of this new field has to be done each time a user declares its interest for a question, i.e. each time a record is added to the Interest table. You could implement that with a trigger in MySQL, but that would be a database dependent solution, and you wouldn't be able to switch to another database as easily.

The best solution is to modify the model by overriding the save() method of the Interest class. This method is called each time an object of class Interest is created. So open the askeet/lib/model/Interest.php file and write in the following method:

public function save($con = null)
{  
    $ret = parent::save($con);
 
    // update interested_users in question table
    $question = $this->getQuestion();
    $interested_users = $question->getInterestedUsers();
    $question->setInterestedUsers($interested_users + 1);
    $question->save($con);
 
    return $ret;
}

The new save() method gets the question related to the current interest, and increments its interested_users field. Then, it does the usual save(), but because a $this->save(); would end up in an infinite loop, it uses the class method parent::save() instead.

Secure the updating request with a transaction

What would happen if the database failed between the update of the Question object and the one of the Interest object? You would end up with corrupted data. This is the same problem met in a bank when a money transfer means a first request to decrease the amount of an account, and a second request to increase another account.

If two request are highly dependent, you should secure their execution with a transaction. A transaction is the insurance that both requests will succeed, or none of them. If something wrong happens to one of the requests of a transaction, all the previously succeeded requests are cancelled, and the database returns to the state where it was before the transaction.

Our save() method is a good opportunity to illustrate the implementation of transactions in symfony. Replace the code by:

public function save($con = null)
{
  $con = Propel::getConnection();
  try
  {
    $con->begin();
 
    $ret = parent::save($con);
 
    // update interested_users in question table
    $question = $this->getQuestion();
    $interested_users = $question->getInterestedUsers();
    $question->setInterestedUsers($interested_users + 1);
    $question->save($con);
 
    $con->commit();
 
    return $ret;
  }
  catch (Exception $e)
  {
    $con->rollback();
    throw $e;
  }
}

First, the method opens a direct connection to the database through Creole. Between the ->begin() and the ->commit() declarations, the transaction ensures that all will be done or nothing. If something fails, an exception will be raised, and the database will execute a rollback to the previous state.

Change the template

Now that the ->getInterestedUsers() method of the Question object works properly, it is time to simplify the _interested_user.php fragment by replacing:

<?php echo count($question->getInterests()) ?>

by

<?php echo $question->getInterestedUsers() ?>

In terms of number of requests and execution time, this should be better. You can verify it with the number of database requests indicated in the web debug toolbar, after the database icon. Notice that you can also get the detail of the SQL queries for the current page by clicking on the database icon itself:

Test the validity of the modification

We'll check that nothing is broken by requesting the show action again, but before that, run again the data import batch that we wrote yesterday:

$ cd /home/sfprojects/askeet/batch
$ php load_data.php

When creating the records of the Interest table, the sfPropelData object will use the overridden save() method and should properly update the related User records. So this is a good way to test the modification of the model, even if there is no CRUD interface with the Interest object built yet.

Check it by requesting the home page and the detail of the first question:

http://askeet/frontend_dev.php/
http://askeet/frontend_dev.php/question/show/id/XX

The number of interested users didn't change. That's a successful move!

Same for the answers

What was just done for the count($question->getInterests()) could as well be done for the count($answer->getRelevancys()). The only difference will be that an answer can have positive and negative votes by users, while a question can only be voted as 'interesting'. Now that you understand how to modify the model, we can go fast. Here are the changes, just as a reminder. You don't have to copy them by hand for tomorrow's tutorial if you use the askeet SVN repository.

• Add the following columns to the answer table in the schema.xml

  <column name="relevancy_up" type="integer" default="0" />
  <column name="relevancy_down" type="integer" default="0" />

• Rebuild the model and update the database accordingly

  $ symfony propel-build-model
  $ symfony propel-build-sql
  $ mysql -u youruser -p askeet < data/sql/lib.model.schema.sql
  

• Override the ->save() method of the Relevancy class in the lib/model/Relevancy.php

  public function save($con = null)
  {
    $con = Propel::getConnection();
    try
    {
      $con->begin();
   
      $ret = parent::save();
   
      // update relevancy in answer table
      $answer = $this->getAnswer();
      if ($this->getScore() == 1)
      {
        $answer->setRelevancyUp($answer->getRelevancyUp() + 1);
      }
      else
      {
        $answer->setRelevancyDown($answer->getRelevancyDown() + 1);
      }
      $answer->save($con);
   
      $con->commit();
   
      return $ret;
    }
    catch (Exception $e)
    {
      $con->rollback();
      throw $e;
    }
  }

• Add the two following methods to the Answer class in the model:

  public function getRelevancyUpPercent()
  {
    $total = $this->getRelevancyUp() + $this->getRelevancyDown();
   
    return $total ? sprintf('%.0f', $this->getRelevancyUp() * 100 / $total) : 0;
  }
   
  public function getRelevancyDownPercent()
  {
    $total = $this->getRelevancyUp() + $this->getRelevancyDown();
   
    return $total ? sprintf('%.0f', $this->getRelevancyDown() * 100 / $total) : 0;
  }

• Change the part concerning the answers in question/templates/showSuccess.php by:

  <div id="answers">
  <?php foreach ($question->getAnswers() as $answer): ?>
    <div class="answer">
      <?php echo $answer->getRelevancyUpPercent() ?>% UP <?php echo $answer->getRelevancyDownPercent() ?> % DOWN
      posted by <?php echo $answer->getUser()->getFirstName().' '.$answer->getUser()->getLastName() ?> 
      on <?php echo format_date($answer->getCreatedAt(), 'p') ?>
      <div>
        <?php echo $answer->getBody() ?>
      </div>
    </div>
  <?php endforeach; ?>
  </div>

• Add some test data in the fixtures

  Relevancy:
    rel1:
      answer_id: a1_q1
      user_id:   fabien
      score:     1
  
    rel2:
      answer_id: a1_q1
      user_id:   francois
      score:     -1
  

• Launch the population batch

• Check the question/show page

Routing

Since the beginning of this tutorial, we called the URL

http://askeet/frontend_dev.php/question/show/id/XX

The default routing rules of symfony understand this request as if you had actually requested

http://askeet/frontend_dev.php?module=question&action=show&id=XX

But having a routing system opens up a lot of other possibilities. We could use the title of the questions as the URL, to be able to require the same page with:

http://askeet/frontend_dev.php/question/what-shall-i-do-tonight-with-my-girlfriend

This would optimize the way the search engines index the pages of the website, and to make the URLs more readable.

Create an alternate version of the title

First, we need a converted version of the title - a stripped title - to be used as an URL. There's more than one way to do it, and we will choose to store this alternate title as a new column of the Question table. In the schema.xml, add the following line to the Question table:

<column name="stripped_title" type="varchar" size="255" />
<unique name="unique_stripped_title">
  <unique-column name="stripped_title" />
</unique>

...and rebuild the model and update the database:

$ symfony propel-build-model
$ symfony propel-build-sql
$ mysql -u youruser -p askeet < data/sql/lib.model.schema.sql

We will soon override the setTitle() method of the Question object so that it sets the stripped title at the same time.

Custom class

But before that, we will create a custom class to actually transform a title into a stripped title, since this function doesn't really concern specifically the Question object (we will probably also use it for the Answer object).

Create a new myTools.class.php file under the askeet/lib/ directory:

<?php
 
class myTools
{
  public static function stripText($text)
  {
    $text = strtolower($text);
 
    // strip all non word chars
    $text = preg_replace('/\W/', ' ', $text);
 
    // replace all white space sections with a dash
    $text = preg_replace('/\ +/', '-', $text);
 
    // trim dashes
    $text = preg_replace('/\-$/', '', $text);
    $text = preg_replace('/^\-/', '', $text);
 
    return $text;
  }
}

Now open the askeet/lib/model/Question.php class file and add:

public function setTitle($v)
{
  parent::setTitle($v);
 
  $this->setStrippedTitle(myTools::stripText($v));
}

Notice that the myTools custom class doesn't need to be declared: symfony autoloads it when needed, provided that it is located in the lib/ directory.

We can now reload our data:

$ symfony cc
$ php batch/load_data.php

If you want to learn more about custom class and custom helpers, read the extension chapter of the symfony book.

Change the links to the show action

In the listSuccess.php template, change the line

<h2><?php echo link_to($question->getTitle(), 'question/show?id='.$question->getId()) ?></h2>

by

<h2><?php echo link_to($question->getTitle(), 'question/show?stripped_title='.$question->getStrippedTitle()) ?></h2>

Now open the actions.class.php of the question module, and change the show action to:

public function executeShow()
{
  $c = new Criteria();
  $c->add(QuestionPeer::STRIPPED_TITLE, $this->getRequestParameter('stripped_title'));
  $this->question = QuestionPeer::doSelectOne($c);
 
  $this->forward404Unless($this->question);
}

Try to display again the list of questions and to access each of them by clicking on their title:

http://askeet/frontend_dev.php/

The URLs correctly display the stripped title of the questions:

http://askeet/frontend_dev.php/question/show/stripped-title/what-shall-i-do-tonight-with-my-girlfriend

Changing the routing rules

But this is not exactly how we wanted them to be displayed. It is now time to edit the routing rules. Open the routing.yml configuration file (located in the askeet/apps/frontend/config/ directory) and add the following rule on top of the file:

question:
  url:   /question/:stripped_title
  param: { module: question, action: show }

In the url line, the word question is a custom text that will appear in the final URL, while the stripped_title is a parameter (it is preceded by :). They form a pattern that the symfony routing system will apply to the links to the question/show action calls - because all the links in our templates use the link_to() helper.

It is time for the final test: Display again the homepage, click on the first question title. Not only does the first question show (proving that nothing is broken), but the address bar of your browser now displays:

http://askeet/frontend_dev.php/question/what-shall-i-do-tonight-with-my-girlfriend

If you want to learn more about the routing feature, read the routing policy chapter of the symfony book.

See you Tomorrow

Today, the website itself didn't get many new features. However, you saw more template coding, you know how to modify the model, and the overall code has been refactored in a lot of places.

This happens all the time in the life of a symfony project: the code that can be reused is refactored to a fragment or a custom class, the code that appears in an action or a template and that actually belongs to the model is moved to the model. Even if this spreads the code in lots of small files disseminated in lots of directories, the maintenance and evolution is made easier. In addition, the file structure of a symfony project makes it easy to find where a piece of code actually lies according to its nature (helper, model, template, action, custom class, etc.).

The refactoring job done today will speed up the development of the upcoming days. And we will periodically do some more refactoring in the life of this project, since the way we develop - make a feature work without worrying about the upcoming functionalities - requires a good structure of code if we don't want to end up with a total mess.

What's for tomorrow? We will start writing a form and see how to get information from it. We will also split the list of questions of the home page into pages. In the meantime, feel free to download today's code from the SVN repository (tagged release_day_4) at:

http://svn.askeet.com/tags/release_day_4/

and to send us any questions using the askeet mailing-list or the dedicated forum.