Request for comments on proposed Extract Method RFE

I don't know about you guys, but I use Extract Method all the time. I've come up with some ideas for improving it, and I wanted you guys feedback on my proposed ideas before I filed an RFE officially.

I want to know if you think you would use / like such a feature, and if you have any ideas for improving it. My description might be complicated and poorly written, I hope you can understand it.

PROBLEM:

I often have complex code from which I want to extract a method consisting of many statements. (This RFE mostly applies to extracting series of statements, not simple single expressions.) The process I use for extracting methods from such code is usually:

1. Introduce Variable for the expression which I want to be the return value of the extracted function

2. Move the variable declaration to the place right after all its necessary values have been computed (so as not to include undesired statements in the extracted method)

3. Select each expression in the to-be-extracted code block, which references local variables / parameters, and:
___a. Inline Variable on the value of the local variable if it was a simple re-computable expression (like calling field getter)
-


or -


___b. Introduce Variable for the expression, if I desire for that value to be passed in as an argument to the exracted method

This is a somewhat simple process now that I have gotten used to it, but I think the Extract Method refactoring would be more useful and powerful and easy to use if this process were automated.

PROPOSED IMPLEMENTATION:

My idea of the user experience for an enhanced Extract Method is as follows. I think the UI visually could be two miniature editor windows: one containing the current code in the editor, and another being a preview of the extracted method. This preview would be updated in real time, reflecting changes made throughout the process described below. There would also be a list of extracted method parameters somewhere in the window.

Each step in this list could be executed as part of a Wizard-style dialog, if the UI were too cumbersome otherwise.

1. User selects region of code which is to be extracted

2. User is prompted to choose an expression in the rest of the method which is the return value:
___a. The editor enters a mode where all expressions which use the value produced by the to-be-extracted method statements are highlighted (when the mouse rolls over, a Hand cursor is shown)
___b. The user chooses a return value by clicking one of the highlighted expression

3. User chooses extracted method parameters:
___a. Editor enters mode where all uses of original method variables/parameters, which will be passed as extracted method's parameters, are highlighted; a list of the parameters (perhaps with Google Toolbar-style color-coded matching highlighting) is shown in a floating/docked dialog
b. User can click one of the highlighted uses, to see a menu with up to three options: "Widen expression" "Inline outer expression" "Inline expression"
i. If user chooses Widen, the enclosing expression is selected (with Ctrl+W semantics), and the parameter now uses that expression instead of the original expression
ii. If user chooses Inline outer, the outermost expression (with CtrlShiftW semantics) is inlined into the method, using the parameter as the inner expression and the parameter which now represents the expression which the original expression contained
iii. If user chooses Inline expression, the entire expression is inlined in the extracted method, based on its last assignment in the original method. This may cause the parameter to be removed, and/or may cause new parameters to be created (if the expression in the last assignment used other local variables/parameters from the original method)
c. User can also select other un-highlighted expression in this mode, and mark the expression to be passed as an argument to the extracted method (user is asked for a name for this parameter)

4. Finally, user clicks "OK" button, and the specified method is extracted

CONCLUSION

I think this would be a great, powerful enhancement to IDEA's refactoring capabilities, requiring less work to extract methods.

What do you think - would you make use of this? Do you "extract methods" very often? Do you have ideas for making my proposed Extract Method process more effective?

8 comments

Could you please provide a code snippet as an example, briefly describing which actions are you currently taking before Extract Method and how an enhanced Extract Method would work? I'm sure this would be useful for many people in order to understand and judge at face value your proposal.

0

Okay, I'll try. Here's an example I came up with:

]]> and list title, converts it to a string "Names (3): A, B, and C".

Currently, to allow Extract Method to extract a method with these semantics, I need to extract and inline some variables and move some declarations around the method until I get this:
 strings = Arrays.asList(array);
        int numStrings = strings.size();
        printText("Processing " + numStrings + " strings");

        String title = getTitle();

        +// Selection starts on this line+
        StringBuilder sb = new StringBuilder(100);
        +// ** CHANGE: inlined numStrings here, it would be silly to pass that as+
        +// **         a parameter when the list itself is being passed+
        +// ** CHANGE: extracted call to getTitle() to a variable, so title will +
        +// **         be a parameter to the extracted method+
        sb.append(title.toUpperCase() + " (" + strings.size() + "): ");

        boolean first = true;
        for (String string : strings) {
            if (first) first = false;
            else sb.append(", ");

            sb.append(string);
        }

        +// ** CHANGE: extracted stringbuffer->string conversion; the method should+
        +// **         return a String+
        String listString = sb.toString();
        +// Selection ends on this line+

        printText("Strings:");
        printText(listString);
    }
]]>

So, this semi-simple method required me to introduce two variables, move their declarations, and inline the value of the list length (I had to inline manually: IDEA will not inline single uses of variables, like it does with methods). Now I can finally Extract Method and it will produce the method I wanted:

 strings = Arrays.asList(array);
        int numStrings = strings.size();
        printText("Processing " + numStrings + " strings");

        String title = getTitle();

        String listString = getListContentsString(title, strings);

        printText("Strings:");
        printText(listString);
    }

    private String getListContentsString(String title, List strings) {
        StringBuilder sb = new StringBuilder(100);
        sb.append(title.toUpperCase() + " (" + strings.size() + "): ");

        boolean first = true;
        for (String string : strings) {
            if (first) first = false;
            else sb.append(", ");

            sb.append(string);
        }
        String listString = sb.toString();
        return listString;
    }
]]>


PROPOSED PROCESS

Let's go back to the original code:

 strings = Arrays.asList(array);
        int numStrings = strings.size();
        printText("Processing " + numStrings + " strings");

        +// Selection starts on this line+
        StringBuilder sb = new StringBuilder(100);
        sb.append(getTitle().toUpperCase() + " (" + numStrings + "): ");

        boolean first = true;
        for (String string : strings) {
            if (first) first = false;
            else sb.append(", ");

            sb.append(string);
        }
        +// Selection ends on this line+

        printText("Strings:");
        printText(sb.toString());
    }

    private String getTitle() {
        return "Title";
    }

    void printText(String s) {
        System.out.println(s);
    }
]]>

With my proposed method, I'd choose Extract Method from Refactoring menu, and IDEA would show me my method, with the following highlighting (I'll explain bold and underline afterwards):

 strings = Arrays.asList(array);
        int numStrings = strings.size();
        printText("Processing " + numStrings + " strings");

        // Selection starts on this line
        StringBuilder sb = new StringBuilder(100);
        sb.append(getTitle().toUpperCase() + " (" + numStrings + "): ");

        boolean first = true;
        for (String string : strings) {
            if (first) first = false;
            else sb.append(", ");

            sb.append(string);
        }
        // Selection ends on this line

        printText("Strings:");
        printText(*_sb_.toString()*);
    }
]]>

The highlighting shows that there are two possible returned expressions in this case. The underlined "sb" would be drawn in some dark blue, and the sb.toString() would be drawn in some lighter blue, to show that the two expressions could both be results of the extracted method, and that one is a part of the other.

On this screen I can roll my mouse over "sb" and see it highlighted. If I click it, then "sb" is set as the return edvalue of the extracted method, and the return type will be StringBuffer. If I clicked the lighter blue ".toString()", the expression "sb.toString()" would be selected as the returned value of the extracted method, and the return type would be String.

In this case, I want "sb.toString()" to be the returned expression of the extracted method, so I click the light blue ".toString()". We move to the next screen.

Next, I am presented with a new screen with the following highlighting:

 strings = Arrays.asList(array);
        int numStrings = strings.size();
        printText("Processing " + numStrings + " strings");

        // Selection starts on this line
        StringBuilder sb = new StringBuilder(100);
        sb.append(getTitle().toUpperCase() + " (" + _numStrings_ + "): ");

        boolean first = true;
        for (String string : _strings_) {
            if (first) first = false;
            else sb.append(", ");

            sb.append(string);
        }
        // Selection ends on this line

        printText("Strings:");
        printText(sb.toString());
    }
]]>

Two local variable usages within the to-be-extracted block are highlighted: numStrings, and strings. These are highlighted because they will need to be passed to the extracted method as arguments. (These are also the arguments that IDEA's current Extract Method refactoring would use as parameters to the extracted methods.)

I only need to modify one of these, but for the purposes of demonstration, I explain that I can click on "strings" to see a popup menu containing only one item: "Inline expression". If I clicked this, it would replace the use of "strings" in the foreach loop, to "Arrays.asList(array)", and now "array" would be highlighted, because it is now required to be a parameter to the extracted function. I could click "array" and see "Widen expression" to revert back to where I was before, with "strings" in the for loop.

I can click on "numStrings" to see a menu with only one item: "Inline expression". If I choose this, the use of "numStrings" is replaced with "strings.size()", and now I have one fewer parameter! This is what I wanted.

Now two of the three changes that I need have been made. We only need to make one more: I want the title of the list to be a parameter to the extracted method. I can do this on this screen by selecting the expression "getTitle()", and clicking to see a menu containing "Add parameter". I click this, and now "getTitle()" is highlighted and will become a parameter to the extracted method.

Finally, this is the method I want, with the parameters I want, and with the returned expression that I want. I can now click "OK" button, and this method is produced:

 strings = Arrays.asList(array);
        int numStrings = strings.size();
        printText("Processing " + numStrings + " strings");

        String listString = getListContentsString(getTitle(), strings);

        printText("Strings:");
        printText(listString);
    }

    private String getListContentsString(String title, List strings) {
        StringBuilder sb = new StringBuilder(100);
        sb.append(title.toUpperCase() + " (" + strings.size() + "): ");

        boolean first = true;
        for (String string : strings) {
            if (first) first = false;
            else sb.append(", ");

            sb.append(string);
        }
        String listString = sb.toString();
        return listString;
    }
]]>

This is, aside from one non-semantic change, the same method that was extracted using the "CURRENT METHOD" described above. That one non-semantic change is that instead of the variable "title", getTitle() is passed directly to the extracted method. In this way, the resulting method is cleaner than the method produced in "CURRENT METHOD" section.

IDEA could not inline the variable listString, however, because it might change the method's semantics to move the call to the extracted method below the first printText call.

CONCLUSION

My proposed enhanced UI extracted the same methods which was produced using current introduce-inline-etc. method. I think using my proposed UI, this was achieved more intuitively and in a more easy-to-use manner (especially for developers who are new to refactoring), without having to chain together three extra refactorings combined with cut-and-paste declaration moves.

I hope this example makes my proposition clearer.

0

I noticed something, when I say "IDEA could not inline the variable listString, however, because it might change the method's semantics to move the call to the extracted method below the first printText call." I was partially wrong.

IDEA is already changing the semantics of the method by moving the sb.toString() call into the extracted method, and before the printText call. (This is a problem because printText() could, in theory, change the result of sb.toString().)

I think IDEA could handle this by warning the user that moving a method call will change the semantics of the method, if such an expression is chosen as the return expression.

0

Keith, your proposal sounds FANTASTIC. At first, it was hard to picture the UI and modus operandis, but the step-by-step example made it sound feasible. This refactoring could be a real highlight in IDEA feature chart.

One caveat, however: maybe there's some value in keeping the current limited, but simple-to-use refactoring. Most of the time people would just be extracting simple expressions, and the current implementation would be good enough.

0


I use extract method all the time, Indeed it's pretty rare for me to actually type a method declaration, rather than simply coding up some functionality and then extract it. Your process for prepping for a method extraction more or less follows mine, although I rarely inline before abstracting (if it was worth making a local variable, it's worth making a parameter). You've identified the issue very well, but I can't help but think your solution is too cumbersome. If I was a new user were faced with the panel you describe, I'd freak. Conversely, as an experienced user, I'd hate a wizard-style interface. I don't have an answer, just a worry, I'm afraid.

--Dave Griffith

0

I really like the idea of a real-time preview, where you can continue editing the original code and see changes in the method signature and body. That would be pretty nice, especially if it helps me to figure out what the return type will be so I can twiddle the code until it's the right type.

However, I think the other features (expression highlighting/choosing) would need polishing in terms of usability and clunkiness.

My usual method is to extract a chunk of code, then use Introduce Parameter, Remove Unused Parameter intention, and both versions of Change Method Signature (the dialog, and the one where you just add parameters to the method call and it updates the signature). Then I use Inline Variable to clean up local variables in the calling method and also in the called method. Maybe an Introduce Field here and there. Lots of Ctrl-W everywhere. Sounds complicated, but it flows pretty well and doesn't require any BDUF, just little incremental changes when I notice an opportunity or need.

0

If I understand this correctly from a cursory glance, it would be awesome.

I often find myself doing the same dance
- want to extract method
- introduce variables and put them above lines to extract so that they get passed in as parameters
- do extract method
- inline the introduced variables

0

Sounds good to me, although I'd have to see the UI to be get a better feeling. Can you create an issue in Youtrack so that we can vote for it?

Thanks,
Dirk

0

Please sign in to leave a comment.