Request for comments on proposed Extract Method RFE

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.

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.

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.

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.

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

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.

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

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