A use case for the “extract method” refactoring

Published on and tagged with refactoring

While refactoring I noticed the following pattern:

public function doSomething() {
    $aVariable = '';

    if (X) {
        $aVariable = 'x';
    } else {
        $aVariable = 'y';
    }

    if ($this->isSomething()) {
        // ...
    } else {
        $anotherVariable = $aVariable . 'z';
        // ...
    }
}

In this example the variable $aVariable is defined at the top of the method, but it is only used in the “else” case of the second “if” statement. So this means the definition of $aVariable happens too early, at a time when it is not clear whether $aVariable is even needed. To fix it, we can move the first “if” block to the “else” case of the second “if”:

public function doSomething() {
    if ($this->isSomething()) {
        // ...
    } else {
        $aVariable = '';

        if (X) {
            $aVariable = 'x';
        } else {
            $aVariable = 'y';
        }

        $anotherVariable = $aVariable . 'z';
        // ...
    }
}

That’s better. But we are not finished yet. If you look at the code, the definition of $aVariable is very dominant, and can distract the reader of the code from the method’s purpose. To improve it, we could use the ternary operator:

$aVariable = X ? 'x' : 'y';

Using the ternary operator is sometimes seen as bad practice (see for example the coding standard for CakePHP). So it is better to apply the Extract Method refactoring:

public function doSomething() {
    if ($this->isSomething()) {
        // ...
    } else {
        $anotherVariable = $this->getAVariable() . 'z';
        // ...
    }
}

private function getAVariable() {
   $aVariable = '';

    if (X) {
        $aVariable = 'x';
    } else {
        $aVariable = 'y';
    }

    return $aVariable;
}

With that, the “doSomething” method has become easier to read and understand.

Happy refactoring :)

16 comments baked

  • Chris Forrette

    This is nice, but I’m big on minimalist code–less is more kind of thing. I tend to reserve function definitions only for when something is repeated–which actually could be the case with whatever you’re working on. In this case, I would exclude the ‘getAVariable’ function unless it was useful somewhere else as well. And I personally love the ternary operator :) Great site by the way! Lots of great examples and explorations with Cake and code in general…

  • Juan Basso

    To be more efficient and otimized:

    private function getAVariable() {
    if (X) {
    return ‘x’;
    }
    return ‘y’;
    }

    But, nice work, Daniel. It’s very more beautiful to the eyes. The sucks is overhead.

  • Grant Cox

    Are you actually suggesting this practice for everywhere you have a simple “if else” on a variable definition – or is this just an overly simple example?

    I can understand the point of this when the chunk you “outsource” is >20 lines, but otherwise if the code is only used in a single location I wouldn’t bother – I personally see that as more messy (multiple small functions calling each other) than one larger self-contained function.

  • titang

    Personally, even if the ternary operator is a bad practice, i prefer to use it instead of to move the code to another function. With the ternary operator you can understand quickly what is the goal of the first function, and you don’t have to search what is the goal of the other function.

  • TariqueSani

    @titang – my thumbrule for using ternary operators is use them if the line length does not go beyond 80 chars

  • Juan Basso

    It’s really Tarique. Some ternary operators is more illegible than other method/function.

    But, I have one question: And… who to make it in view? Use helpers “generics”?

    Daniel: Can you write about something cool in views? It’s great too.

  • cakebaker

    @all: Thanks for your comments!

    @Chris: I use methods not only for code that’s used multiple times, but also to improve the readability of my code by hiding details. In the end I strive for minimalist code like you, just using a different approach ;-)

    @Grant: I would use it everywhere (if possible). But of course, it is a matter of preference, not everyone likes to have a lot of small methods ;-)

    @titang: If you use the ternary operator carefully (see Tarique’s comment), it is a very useful construct.

    @Juan: Hm, I don’t understand your question: “who to make it in view? Use helpers generics?”

    And regarding your other question: do you have something special in mind you want to read here?

  • Juan Basso

    When we need use ternary operators or conditions in views… What we do?

    For example, I have a XML-RPC method. When it’s work fine and have results, I show them. But, sometimes it fail (because network, protocol, other…). How to display? If…else… or elements?

    My question is about conditions in views.

  • Grant Cox

    Definitely if-else in views – as your elements are each in separate files, and have no concept of “private” like these outsourced functions can. As such you’d end up with dozens of “public” elements, it would be very difficult to tell them apart, and understanding what a view is rendering would be such a pain (having to open up every separate element file to see what’s actually happening).

  • cakebaker

    @Juan: As Grant already mentioned, using if-else is almost always the best way to deal with such a scenario. And if your error output becomes more complex, you can still move it to an element like:

    <?php if ($error): ?>
        <?php echo $this->element('error'); ?>
    <?php else: ?>
        Your main content
    <?php endif; ?>
    

    Hope that helps!

  • Nico Granelli

    I’m really happy to see some Refactoring evangelization.

    Only one comment: replace “getAVariable” for something more related to the domain, like “chooseSomthing”

    It’s very important to extract methods all around, even if not will be called in other place. I like to get High Cohesive Classes, so after extracting some methods, I create a new class, and move all the new methods to the new class.

  • cakebaker

    @Nico: Thanks for your comment!

    Yes, you are right, the method name “getAVariable” is a bit unlucky (in hindsight I should have used a real example…).

  • George

    I’m all for writing readable code, but this is too much.

    Extract Method is great when I don’t care what’s happening in the function. But in the case of a variable assignment, I want to know what the variable is being set to and based on what conditions. The ternary operator achieves this and still produces very clean code.

    I don’t find simple applications of the ternary operator hard to read in the least. After all, anyone reading the code is a developer.

  • cakebaker

    @George: *g*, I know, the example is not that good, and using the ternary operator would be enough in that case ;-)

    In the real use case, I had a path variable, and its value depended on the setting of a (boolean) constant. With Extract Method I can hide this detail: I just call getImagePath(), and I get the correct path.

  • Sebastian

    @Daniel: very nice article. I came here searching for a refactoring tool in php with a “extract method” tool like in eclipse, but sadly this doesn’t seem to exist.

    @Chris&Co: It is general knowledge that small methods are good, even for only one line of code! Read Martin Flowlers “refactoring”, try to program this way, and you will see the difference and become a better programmer.

  • cakebaker

    @Sebastian: Thanks for your comment!

    Yes, as far as I know there is no tool for PHP (yet) that provides the “extract method” refactoring (or any other more advanced refactoring)…

Bake a comment




(for code please use <code>...</code> [no escaping necessary])

© daniel hofstetter. Licensed under a Creative Commons License