I am a programmer and architect (the kind that writes code) with a focus on testing and open source; I maintain the PHPUnit_Selenium project. I believe programming is one of the hardest and most beautiful jobs in the world. Giorgio is a DZone MVB and is not an employee of DZone and has posted 638 posts at DZone. You can read more from them at their website. View Full User Profile

Practical PHP Refactoring: Add Parameter

11.07.2011
| 3717 views |
  • submit to reddit

In the scenario of today, a method is missing some information to accomplish its responsibility. Usually, it is forced to look up these data into some form of global state (like a singleton) or it just avoids to support some use case because of the missing piece it needs.

In the first case (the second would be an unacceptable missing functionality), there is a problem with looking up information; outward dependencies are established from this class to other parts of the system. You are not able to reuse or test the current class without having Zend_Controller_Front ready, or some $_SESSION variables set.

An alternative is to pass the reference as a method parameter. Fowler underlines that it's just a slightly better option, and there will be more in the next articles of this series where we will explore the Making method calls simpler part of his book.

Why another parameter?

You need to provide the object with that dependency in some way: otherwise the object cannot work, or has to copy some code from another class, introducing duplication.

If the dependency is needed only in that particular method, keeping it in a private field makes less sense. Equivalently, if the same object operates against many different instances of this dependency, a field makes even less sense.

Why not yet another parameter?

Parameters lists tend to become longer and longer: PHPUnit_Framework_TestCase::getMock() is an example of a popular method accepting 7 parameters (although most of them are optional.) A parameter also introduce some dependencies: the object get dependent on the parameter even if it's just referenced in one of its methods and not in its other 100 lines of code.

If the object is not able to satisfy a method call without additional parameters, it may also be the case that this responsibility is not to be placed upon it. Sometimes the responsibility can be broken up, and the business logic divided into into an existing parameter or collaborator, and the current object. In that case, accessing the new parameter is a concern outsourced to another object which may already have it readily available in a field or on the stack.

But let's stop this abstract talk and get into some code!

Steps

As a prerequisite, check for subclasses and superclasses: if you want to maintain a polymorphic behavior, all copies of the method in the hierarchy have to maintain a common signature. You will have to add the parameter to them too.

  1. Add a new method, with the new parameter. Copy in it the code like for a renaming operation, and use a slightly different name (see the example to discover why).
  2. Change the old copy of the method so that it just delegates to the new one. Especially when the older method probably cover many use cases, we don't want to break them; once the two methods are working and the duplication has been eliminated, we can refactor the calls one at the time.
  3. If the tests are passing, change the calls to the old method and continue to keep the suite green.
  4. After having changed all the calls, delete the old version of the method as it should not be reached anymore neither from production code nor from the test suite.

Example

In the initial state, we are describing a fiscal system similar to the Italian one, where invoices must all have progressive numbers assigned according to their date.

However, an invoice can be inserted in the middle of the list in some cases, since there is no requirement to register invoices in the computer system in their actual chronological order.

<?php
class AddParameter extends PHPUnit_Framework_TestCase
{
public function testProvidesNextInvoiceNumbersForTheCurrentDate()
{
$invoices = new Invoices(array(1 => '2011-10-01', 2 => '2011-11-01'));
$this->assertEquals(3, $invoices->getNextProgressiveNumber());
}

public function testNextInvoiceNumberIsAnExistingOneInCaseWeHaveToRenumerate()
{
$invoices = new Invoices(array(1 => '2011-10-01', 2 => '2011-11-10'));
$this->assertEquals(2, $invoices->getNextProgressiveNumber());
}
}

class Invoices
{
private $invoiceDates;

public function __construct($invoiceDates)
{
$this->invoiceDates = $invoiceDates;
}

public function getNextProgressiveNumber()
{
$currentDate = date('Y-m-d');
foreach ($this->invoiceDates as $number => $date) {
if ($date > $currentDate) {
$nextNumber = $number;
break;
}
}
if (!isset($nextNumber)) {
$nextNumber = count($this->invoiceDates) + 1;
}
return $nextNumber;
}
}

The current date is used as the parameter: we would like to pass it from the outside to avoid relying on global state (the system's hardware clock). This would also allow us to perform the first step for the registration of an invoice (getting the valid progressive number) even if the invoice's date is not today.

So we add a new version of the method, accepting the date as a parameter: we have to use a different name as the PHP language does not support method overloading via multiple signatures.

class Invoices
{
private $invoiceDates;

public function __construct($invoiceDates)
{
$this->invoiceDates = $invoiceDates;
}

public function getNextProgressiveNumber()
{
$currentDate = date('Y-m-d');
foreach ($this->invoiceDates as $number => $date) {
if ($date > $currentDate) {
$nextNumber = $number;
break;
}
}
if (!isset($nextNumber)) {
$nextNumber = count($this->invoiceDates) + 1;
}
return $nextNumber;
}

public function getProgressiveNumberForInsertion($currentDate)
{
foreach ($this->invoiceDates as $number => $date) {
if ($date > $currentDate) {
$nextNumber = $number;
break;
}
}
if (!isset($nextNumber)) {
$nextNumber = count($this->invoiceDates) + 1;
}
return $nextNumber;
}
}

Next, we delegate the old method's responsibility to the new one accepting the parameter.

class Invoices
{
private $invoiceDates;

public function __construct($invoiceDates)
{
$this->invoiceDates = $invoiceDates;
}

public function getNextProgressiveNumber()
{
$currentDate = date('Y-m-d');
return $this->getProgressiveNumberForInsertion($currentDate);
}

public function getProgressiveNumberForInsertion($currentDate)
{
foreach ($this->invoiceDates as $number => $date) {
if ($date > $currentDate) {
$nextNumber = $number;
break;
}
}
if (!isset($nextNumber)) {
$nextNumber = count($this->invoiceDates) + 1;
}
return $nextNumber;
}
}

Now we can modify the client code to use the new method; in the case of our tests, they become real unit tests since they are not based anymore on time as global state.

<?php
class AddParameter extends PHPUnit_Framework_TestCase
{
    public function testProvidesNextInvoiceNumbersForTheCurrentDate()
    {
        $invoices = new Invoices(array(1 => '2011-10-01', 2 => '2011-11-01'));
        $this->assertEquals(3, $invoices->getProgressiveNumberForInsertion('2011-11-02'));
    }

    public function testNextInvoiceNumberIsAnExistingOneInCaseWeHaveToRenumerate()
    {
        $invoices = new Invoices(array(1 => '2011-10-01', 2 => '2011-11-10'));
        $this->assertEquals(2, $invoices->getProgressiveNumberForInsertion('2011-11-02'));
    }
}

Finally, we remove the older version of the method as it is no longer called.

class Invoices
{
private $invoiceDates;

public function __construct($invoiceDates)
{
$this->invoiceDates = $invoiceDates;
}

public function getProgressiveNumberForInsertion($currentDate)
{
foreach ($this->invoiceDates as $number => $date) {
if ($date > $currentDate) {
$nextNumber = $number;
break;
}
}
if (!isset($nextNumber)) {
$nextNumber = count($this->invoiceDates) + 1;
}
return $nextNumber;
}
}
Published at DZone with permission of Giorgio Sironi, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)