extract method refactoring with assignment

Hi All,

    I tried an extract to method refactoring yesterday that failed.  The method looked like

    | ... v1 v2 v3 |
    ....
    v1 := self m1.
    v2 := self m2.
    v3 := v3 collect: [:e| ...].
    ....

such that v1 and v2 were only used in the block, and the block was complex enough and did something useful e Pugh that I wanted to refactor as


| ... v1 v2 v3 |
    ....
    v3 := self complexCollect: v3.
    ....

complexCollect: a
    | v1 v2 |
    v1 := self m1.
    v2 := self m2.
    ^a collect: [:e| ...]

So I was forced by the fact that assignment is written with the variable on the LHS to include "v3 := " in the selection to extract to method, even though my intent was for it not to be included.  And of course the refactoring attempt fails precisely because the assignment /is/ included :-)

Shouldn't this case be specially handled do that and trailing assignment is left behind in the refactored original method?  Ifso, how would one go about it?

Note that part of the prerequisite checks should also check that the variables v1 & v2 are not used elsewhere in the origins nail method and whether the refactoring is smart enough to remove them from the refactored origins nap is optional but definitely preferred.

_,,,^..^,,,_ (phone)

When you extract multiple statements and the last one is an assignment, it should ask if you want to extract the assignment also. Depending on what you select the analysis is a bit different. For example, consider this method:

foo
        | a b |
        a := self printString. “Statement 1"
        b := 1. “Statement 2"
        a := a “Statement 3"
                inject: 0
                into: [ :s :e |
                        b := b + 1.
                        s + b + e asInteger ].
        ^ a “Statement 4"

If you select statements 2 & 3 and do an extract, it should ask if you want to extract the assignment. If you don’t, then you get something like what you wanted above:

foo
        | a |
        a := self printString.
        a := self bar: a.
        ^ a
bar: a
        | b |
        b := 1.
        ^ a
                inject: 0
                into: [ :s :e |
                        b := b + 1.
                        s + b + e asInteger ]

If you do extract the assignment, it isn't allowed since we are splitting the “a” variable (one in #foo and one in #bar). With a little bit of analysis we might be able to determine that the “a” in #foo isn’t live when we are running #bar, but if “a” was used in a block, then things get really tough for that type of analysis. If you do this in VW, you get an error message dialog saying what the problem is. However, if you run in Pharo, you get a notice that gets displayed rather quickly and the refactoring proceeds like everything is fine even though it is producing code that will not run.

If you extract statements 1-3 without the assignment, it sees that you are assigning “a” inside the extracted method and using it in the unextracted code, and also you are needing to return the result of the #inject:into:. Since it appears that we need to return two items, it throws an error. In VW, this error causes a dialog to appear with what is wrong, and the refactoring aborts. In Pharo, the information quickly appears and the refactoring continues (which throws an unexpected error that we are trying to add another return statement after an existing return statement). In this case we could do some analysis to see that we don’t need to return the “a” variable since it is immediately assigned.

Finally, if you extract statements 1-3 with the assignment, you should get:

foo
        | a |
        a := self bar.
        ^ a
bar
        | a b |
        a := self printString.
        b := 1.
        a := a
                inject: 0
                into: [ :s :e |
                        b := b + 1.
                        s + b + e asInteger ].
        ^ a

In this case, you didn’t need to return the value of #inject:into: so it could return the “a” variable as the single return value from the extracted method.


Anyway, Pharo should be fixed to not resume errors that the refactorings are throwing. There is a difference between error and warning.


John Brant