Agile Zone is brought to you in partnership with:

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 637 posts at DZone. You can read more from them at their website. View Full User Profile

Practical PHP Refactoring: Inline Method

06.15.2011
| 6128 views |
  • submit to reddit

Code changes often, to accomodate new requirements or redefined ones. Sometimes a method becomes very short, or obvious; sometimes it tries to accomodate something that is not its responsibility. Eliminating unnecessary abstractions is as important as introducing the right ones, and may be a prerequisite for doing so: Inline Method consists in substituting a method call with the body of the method itself.

In any case, we learn today an universal rule (stated also by Kent Beck in his Responsive Design presentation): all refactorings are bidirectional, and in some cases we follow one direction, while in others we go back.

Why inlining a method?

A first reason for inlining a method is that it introduces too much indirection, without abstracting away details. If a method just concatenates two strings, or makes a sum, or assigns a default value in case the return is null, we may get rid of the indirection and just inline its code.

Note that often Inline Method is performed not because the method was too short, but because it has become too short. All refactorings may be taken into consideration not because our codebase is inherently bad and rotting away, but because of other changes that have left the code like this.

Another reason for inlining is a badly factored method, and we'll see this case also in the code sample. If a method performs the wrong subset of actions, you may just want to inline everything in a single, longer method and then perform extraction again.

Steps

  • 0. A prerequisite for this refactoring is checking that the method is not overridden in subclasses. Usually Inline Method is performed over private methods: the code stays in the same class, and there is no problem to check since you only have to fix calls in the current source file.
  • 1. Find all calls and replace them with the method body; remember, it was a short method, so it must not be difficult.
  • 2. Fix references: method parameters should now be local variables; the same goes for the return value, if any is present.

You may also check that internal variables of the method do not conflict with local ones around the old method call, but this would be rare, especially if the inlined method is really a short one.

Fowler suggests that if you encounter difficulties, like recursive methods or different return points, it is a sign that this refactoring won't be effective at this time. Other refactorings may be performed in a Mikado-like order, or the method has just to be revised in name and responsibilities but not eliminated (for now).

Example

We go the other way with respect to the previous example, inlining the method for obtaining the date. Why we do so? To fix a bad division of responsibilities, arised from a change in the specification of this class.

The requirements change is expressed as a change in the test.

<?php
class InlineMethodTest extends PHPUnit_Framework_TestCase
{
    public function testMethodExtractionShouldNotMakeThisTestFail()
    {
        $logParser = new LogParser();
        $logLine = '127.0.0.1 - - [04/30/2011:17:07:31 +0200] "GET /favicon.ico HTTP/1.1" 404 450 "-" "Mozilla"';
        $day = $logParser->getDayOfTheWeek($logLine);
        $this->assertEquals('On Saturday we got a visit from 127.0.0.1', $day);
    }
}

class LogParser
{
    public function getDayOfTheWeek($logLine)
    {
        $date = $this->getDate($logLine);
        return 'On ' . $date->format('l') . ' we got a visit';
    }

    /**
     * @return DateTime
     */
    private function getDate($logLine)
    {
        preg_match('([0-9]{2}/[0-9]{2}/[0-9]{4})', $logLine, $matches);
        return new DateTime($matches[0]);
    }
}

We temporarily bring the test back to refactor: always refactor in a green state if you're not very confident. A green bar is easier to read and maintain in its state that a red bar with only 1 currently failing test.

<?php
class InlineMethodTest extends PHPUnit_Framework_TestCase
{
    public function testMethodExtractionShouldNotMakeThisTestFail()
    {
        $logParser = new LogParser();
        $logLine = '127.0.0.1 - - [04/30/2011:17:07:31 +0200] "GET /favicon.ico HTTP/1.1" 404 450 "-" "Mozilla"';
        $day = $logParser->getDayOfTheWeek($logLine);
        $this->assertEquals('On Saturday we got a visit' /* from 127.0.0.1' */, $day);
    }
}

After having inlined the method, in two simple steps (the Inline Method refactoring stops here) the code looks like this.

<?php
class InlineMethodTest extends PHPUnit_Framework_TestCase
{
    public function testMethodExtractionShouldNotMakeThisTestFail()
    {
        $logParser = new LogParser();
        $logLine = '127.0.0.1 - - [04/30/2011:17:07:31 +0200] "GET /favicon.ico HTTP/1.1" 404 450 "-" "Mozilla"';
        $day = $logParser->getDayOfTheWeek($logLine);
        $this->assertEquals('On Saturday we got a visit' /* from 127.0.0.1' */, $day);
    }
}

class LogParser
{
    public function getDayOfTheWeek($logLine)
    {
        preg_match('([0-9]{2}/[0-9]{2}/[0-9]{4})', $logLine, $matches);
        $date = new DateTime($matches[0]);
        return 'On ' . $date->format('l') . ' we got a visit';
    }
}

It's not part of the refactoring of today, but we add again the test and make it pass. We may perform now a new Extract Method if there is too much logic here, for example a method to contain the regex and that returns an associative array with the parsed information. It would probably be more robust than the original getDate() to future change, that require to parse other parts of the log line.

<?php
class InlineMethodTest extends PHPUnit_Framework_TestCase
{
    public function testMethodExtractionShouldNotMakeThisTestFail()
    {
        $logParser = new LogParser();
        $logLine = '127.0.0.1 - - [04/30/2011:17:07:31 +0200] "GET /favicon.ico HTTP/1.1" 404 450 "-" "Mozilla"';
        $day = $logParser->getDayOfTheWeek($logLine);
        $this->assertEquals('On Saturday we got a visit from 127.0.0.1', $day);
    }
}

class LogParser
{
    public function getDayOfTheWeek($logLine)
    {
        preg_match('/([0-9.]*) (.*)([0-9]{2}\/[0-9]{2}\/[0-9]{4})/', $logLine, $matches);
        $ip = $matches[1];
        $date = new DateTime($matches[3]);
        return 'On ' . $date->format('l') . ' we got a visit from ' . $ip;
    }
}
 
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.)