Practical PHP Refactoring: Pull Up Method

We are in the part of the series where refactorings target mostly the elimination of duplicated code. For now, most solutions will achieve this goal via inheritance.

The Pull Up Method refactoring identified a duplicated method that reside in multiple subclasses and factors it out in an existing common base class; extracting the common class is the job of other refactorings.

Similarly to the case of the Pull Up Field refactoring, the copies of the method may not be identical at first. We can adopt this definition: two methods are duplicated if they produce the same results for all the tests (which should cover the same test cases for all the involved subclasses.)

With this definition in mind, the refactoring will be stopped by the tests in case the methods are actually different for some corner case. If you can execute preliminary refactorings that makes the copies of the method actually identical, do it and then come back here: it will be simpler to continue.

Why?

Inheritance has always been one of the simplest way to share common code. Whenever a method is pulled up, the client code does not have to change. It is actually too simple sometimes: we will consider delegation instead of inheritance in the next episodes.

Inheritance-based elimination of duplication can become almost a reflex, especially if we just name a class AbstractXxxxx and pull up everything. Delegation takes more work, but produces better results as new concepts are discovered and methods are effectively hidden behind a new object instead of being left in the same API.

Thus inheritance is one of our weapons (along with surprise, fear and ruthless efficiency) for eliminating duplicated code in source files; but sometimes it is just an interpreter-assisted copy and paste: all the subclasses still have the duplicated method when an object is instantiated, but at least it is generated from a single copy in the source code. In many cases you will have to test the method at least twice.

Steps

1. Ensure that the methods are actually identical. Ensure signatures are also the same: the number of parameters, their meaning and their names should be identical across all involved subclasses.
2. Create a new method in the common superclass and copy in it the method body.
3. Delete one subclass method at the time, running the tests to check after each.

Fowler notes that sometimes one of the required types of methods accepting instances of the subclasses can become the superclass. This is actually one of the few type checks we can perform in PHP, type hints:

public function clientCodeMethod(MySubClassObject $object)

may become (not mandatory)

public function clientCodeMethod(MyBaseClassObject $object)

in the case clientCodeMethod() calls just the pulled up methods.

Example

We restart from the end of the Pull Up Field example: we have unified the $author field definition but there is still duplicated code between the two subclasses. This time, we will try to eliminate the multiple versions of __toString(), which are very similar.

<?php
class PullUpMethod extends PHPUnit_Framework_TestCase
{
    public function testAPostShowsItsAuthor()
    {
        $post = new Post("Hello, world!", "giorgiosironi");
        $this->assertEquals("Hello, world! -- giorgiosironi",
                            $post->__toString());
    }

    public function testALinkShowsItsAuthor()
    {
        $link = new Link("http://en.wikipedia.com", "giorgiosironi");
        $this->assertEquals("<a href=\"http://en.wikipedia.com\">http://en.wikipedia.com</a> -- giorgiosironi",
                            $link->__toString());
    }
}

abstract class NewsFeedItem
{
    /**
     * @var string  references the author's Twitter username
     */
    protected $author;
}

class Post extends NewsFeedItem
{
    private $text;

    public function __construct($text, $author)
    {
        $this->text = $text;
        $this->author = $author;
    }

    public function __toString()
    {
        return "$this->text -- $this->author";
    }
}

class Link extends NewsFeedItem
{
    private $url;

    public function __construct($url, $author)
    {
        $this->url = $url;
        $this->author = $author;
    }

    public function __toString()
    {
        return "<a href=\"$this->url\">$this->url</a> -- $this->author";
    }
}

First of all, we have to ensure the two methods to unify are identical. We have to extract a method encapsulating their differences and transform __toString() in a simple Template Method.

class Post extends NewsFeedItem
{
    private $text;

    public function __construct($text, $author)
    {
        $this->text = $text;
        $this->author = $author;
    }

    private function displayedText()
    {
        return $this->text;
    }

    public function __toString()
    {
        return $this->displayedText() . " -- $this->author";
    }
}

class Link extends NewsFeedItem
{
    private $url;

    public function __construct($url, $author)
    {
        $this->url = $url;
        $this->author = $author;
    }

    private function displayedText()
    {
        return "<a href=\"$this->url\">$this->url</a>";
    }

    public function __toString()
    {
        return $this->displayedText() . " -- $this->author";
    }
}

Now __toString() can be pulled up: let's create it in the superclass and leave it shadowed for now. Like in the case of Pull Up Field, we can provide here the documentation for the method. Since __toString() uses an hook method, we also define it here as abstract, although it has nothing to do with this refactoring. The displayedText() implementations also have to become protected.

abstract class NewsFeedItem
{
    /**
     * @var string  references the author's Twitter username
     */
    protected $author;

    /**
     * @return string   an HTML printable version
     */
    public function __toString()
    {
        return $this->displayedText() . " -- $this->author";
    }

    /**
     * @return string
     */
    protected abstract function displayedText();
}

class Post extends NewsFeedItem
{
    private $text;

    public function __construct($text, $author)
    {
        $this->text = $text;
        $this->author = $author;
    }

    protected function displayedText()
    {
        return $this->text;
    }

    public function __toString()
    {
        return $this->displayedText() . " -- $this->author";
    }
}

class Link extends NewsFeedItem
{
    private $url;

    public function __construct($url, $author)
    {
        $this->url = $url;
        $this->author = $author;
    }

    protected function displayedText()
    {
        return "<a href=\"$this->url\">$this->url</a>";
    }

    public function __toString()
    {
        return $this->displayedText() . " -- $this->author";
    }
}

We eliminate the first redefinition from Post:

class Post extends NewsFeedItem
{
    private $text;

    public function __construct($text, $author)
    {
        $this->text = $text;
        $this->author = $author;
    }

    protected function displayedText()
    {
        return $this->text;
    }
}

And the second one from Link:

class Link extends NewsFeedItem
{
    private $url;

    public function __construct($url, $author)
    {
        $this->url = $url;
        $this->author = $author;
    }

    protected function displayedText()
    {
        return "<a href=\"$this->url\">$this->url</a>";
    }
}

The two classes contain (almost) no duplicate code now. We could go on and do the same with the constructor, but since it just assigns $author to a field it's probably not worth to extract and pull up the operation.