Refactoring 2: Extract Method

The ‘Extract Method’ is one of the most common refactorings you will ever do. It is also one you will frequently see implemented on the Refactor tool menu on various IDE’s.

What this method basically does is to take a group of related code and convert it to a function with a appropriate name that easily explains the purpose of the code. You can also use ‘Extract Method’ if one of your methods is too long or requires copious amount of comments to explain its purpose.

Take for example the following code from a recent project. The code converts a hotel room price to points depending on the currency. The code by itself doesn’t do much to explain what it does.

Note: In view of the recent constructive comments given by readers I’ve changed the code to more correctly reflect the concept of the Extract method. Its people like you that always keep me alert. Thanks guys.

<?php
 
function getRoomPoints($roomPrice)
{
    /* $forex is an array containing exchange rates */
    $forex = array("gbp"=>1.2, "eur"=>.75, "usd"=>1);
    $_factor = 0;
    $currency_code = "GBP";
 
    switch($currency_code)
    {
        case "GBP" : $_factor = 1/$forex['gbp'];
                     break;
        case "USD" : $_factor = 1;
                     break;
        case "EUR" : $_factor = 1/$forex['eur'];
                     break;
        default : $_factor = 1;
                     break;
    }
 
    return $roomPrice * $_factor;
}
 
$room_points = getRoomPoints($_service_hotel['AvailableRoom']['Price']);
 
?>

The block of code that does the actual conversion can be extracted and converted into a function with an appropriate name as given below which now makes the intention of the code clear.

<?php
 
function getRoomPoints($roomPrice)
{
    $currency = $_service_hotel['Currency']['code'];
    $_factor =  getConversionRateFactor($currency);
    return $roomPrice * $_factor;
}
 
function getConversionRateFactor($currency_code)
{
    $_factor = 0;
    $forex = array("gbp"=>1.2, "eur"=>.75, "usd"=>1);
 
    switch($currency_code)
    {
        case "GBP" : $_factor = 1/$forex['gbp'];
                     break;
        case "USD" : $_factor = 1;
                     break;
        case "EUR" : $_factor = 1/$forex['eur'];
                     break;
        default : $_factor = 1;
                     break;
    }
 
    return $_factor;
}
 
 
$room_points = getRoomPoints($_service_hotel['AvailableRoom']['Price']);
 
?>

10 thoughts on “Refactoring 2: Extract Method

  1. Not to be confused with the “true” extract method:

    extract($_POST);

    Which is the utmost evil thing to leave behind for fellow developers, be warned.

  2. In an effort to reduce the code size while being more extensible and – as I think – more readable: http://paste.mootools.net/f8819e01

    Just wrapping existing code in a function is not refactoring as this changes the external behaviour (it provides a new function to “the outside” and you have to replace all occurences of currency calculation) – I know it is only an example, but this is not refactoring and more of reworking or so.

    Refactoring, or the “Extract Method” you describe would be when you have a method getPrice that retrieves the conversion factor AND calculates the price based on the currency and you want to move the retrieval of the conversion factor to a new function getConversionRateFactor and call it within getPrice. This way you won’t have to change the surrounding code as getPrice still behaves the same. I know your example aims to be basic, but I think it is more clear to what it is about 🙂

  3. Or you could just do this:

    function getConversionRateFactor($currency_code)
    {
    $forex = array(“gbp”=>1.2, “eur”=>.75, “usd”=>1);
     
    switch($currency_code)
    {
    case “GBP” : return 1/$forex[‘gbp’];
    case “USD” : return 1;
    case “EUR” : return 1/$forex[‘eur’];
    default : return 1;
    }
    }

  4. When I came across the title of this post, I truly was expecting a post about Refactoring but this isn’t refactory (see M Fowler book), and certainly this can’t be seen as the Extract Method either.

    With all the hype going about best practices, etc you’ve jumped on the bandwagon (ie keyword Refactoring) to get a higher page index which isn’t really the case, as advertised is it?

    Disappointed 🙁

  5. Thanks for writing.

    I did learn something from Chris’s comment: that’s not really refactoring.

    But it is restructuring, which can really help improve code readability.

    Please continue this series, and don’t be afraid to make mistakes. If you’re lucky, people will actually read your articles and catch your mistakes.

    Given you came up on the first page on Google, you’re doing the php community a valuable service as few others see fit to write about refactoring. Just make sure you keep it simple enough for non-experts to understand, and as accurate as you know how. Personally, I almost have to write to learn, and I make mistakes along the way. Some people harsh me for mistakes, others point them out so I can learn better.

  6. I don’t really understand why you use a switch in this example, I wouldn’t use it in this case.
    In my opinion this is more dynamic:

    if (isset($forex[strtolower($currency_code)])) {
    $_factor = 1 / $forex[strtolower($currency_code)];
    }
    else {
    $_factor = 1;
    }

    return $_factor;

    Or just in 1 line:

    return isset($forex[strtolower($currency_code)]) ? 1 / $forex[strtolower($currency_code)] : 1;

    You can even drop the strtolower() if you change the keys in $forex to capitals.

  7. We can make the code as terse as we like, but as I had created the code for one of my clients (they are developers) I choose clear expression over brevity.

Comments are closed.