Practical PHP Refactoring: Push Down Method

In the scenario of today, some logic is in a superclass of a hierarchy, modelled as a method. However, this method is only useful for one of the subclasses in the hierarchy.

The proposed solution is to push down the method in the chosen subclass until each class contains inherits only the strictly necessary code.

Why pushing down?

A situation calling for pushing down methods often happens after having split and extracted methods. We assume the logic interesting for a single subclass is represented as a method; if it's not, it can be refactored to assume this form (e.g. a method can be extracted from the existing code, leading to Template Method or Factory Method.)
You can choose what to do basing on the particular situation; if the method is useful for

• all the subclasses, the base class is the right place.
• More than one subclass, it means that an intermediate subclass is needed to contain it. However, this would be more than what this refactoring aims for.
• A single subclass, this refactoring serves you well.
• For no subclass, you should ask why it is there at all. At this point you can usually just delete the unused code, as you will always be able to extract it from source control in case of need.

Steps

The first part of this refactoring is bringing the method down by copying it:

1. Declare a method in all subclasses, copying the body of the original one.
2. Remove the method from the superclass.
3. Check that all tests pass.

The second part is about deleting copies of the method:

1. Remove the method from all the subclasses where it is not needed, one at the time
2. Check that the tests pass after each new intervention.

Some type hints where the method is called should now change from Superclass to ChosenSubclass, as it's not possible to call such method on every instance of Superclass. The tests won't catch a type hint looser than necessary, so it's your job to ensure ChosenSubclass is substituted where possible.

Example

We restart from the hierarchy of the previous examples on Pull Up refactorings, where the Post and Link classes inherit from NewsFeedItem. This time we have a new requirement: only a local link is allowed to avoid spam; this requirement is satisfied with a checkLocality($url) method.

<?php
class PullUpConstructorBody 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("/posts/php-refactoring", "giorgiosironi");
        $this->assertEquals("<a href=\"/posts/php-refactoring\">/posts/php-refactoring</a> -- @giorgiosironi",
                            $link->__toString());
    }

    /**
     * @expectedException InvalidArgumentException
     */
    public function testOnlyLocalLinksAreAllowed()
    {
        $link = new Link("http://www.google.com", "giorgiosironi");
    }
}

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

    public function __construct($author)
    {
        $this->author = '@' . ltrim($author, '@');
    }

    protected function checkLocality($url)
    {
        if (strstr($url, "http") == $url) {
            throw new InvalidArgumentException("The URL '$url' is related to an external website.");
        }
    }

    /**
     * @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)
    {
        parent::__construct($author);
        $this->text = $text;
    }

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

class Link extends NewsFeedItem
{
    private $url;

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

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

However, we notice the method is required only by one of the two subclasses, and we try to move it there to simplify the base class.

Keep in mind these classes could be hundreds of lines long, and performing method calls with __call(); or the calls could be on a public method and thus difficult to identify just by looking at the code *in* the class hierarchy. With real code, we need small steps and we are learning them on this toy example.

We copy the method into each subclass: this change shouldn't change any behavior as they simply override the original copy with itself.

class Post extends NewsFeedItem
{
    private $text;

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

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

    protected function checkLocality($url)
    {
        if (strstr($url, "http") == $url) {
            throw new InvalidArgumentException("The URL '$url' is related to an external website.");
        }
    }
}

class Link extends NewsFeedItem
{
    private $url;

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

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

    protected function checkLocality($url)
    {
        if (strstr($url, "http") == $url) {
            throw new InvalidArgumentException("The URL '$url' is related to an external website.");
        }
    }
}

Then, we start deleting code, which is the fun part. First, from the base class: the test passes, so there isn't any missing extends.

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

    public function __construct($author)
    {
        $this->author = '@' . ltrim($author, '@');
    }

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

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

Then from Post, the first subclass.

class Post extends NewsFeedItem
{
    private $text;

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

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

Now from Link, the other subclass. The tests fail, so we cannot delete the method, which will remain in this single leaf subclass.

class Link extends NewsFeedItem
{
    private $url;

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

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

    protected function checkLocality($url)
    {
        if (strstr($url, "http") == $url) {
            throw new InvalidArgumentException("The URL '$url' is related to an external website.");
        }
    }
}