Inline temp refactoring can create code with different behavior.

[jecisc]

Describe the bug I used "inline temp" refactoring on this source code:

exportOnStream: aStream model: aModel
    | writter |
    writter := FamixSTONWriter on: aStream.
    writter model: aModel.
    writter
        writeObject:
            (FamixSTONExport new
                entities: aModel entities;
                categories: aModel allCategories;
                associations: self allAssociations;
                yourself)

I was expecting this result:

exportOnStream: aStream model: aModel
    (FamixSTONWriter on: aStream)
        model: aModel;
        writeObject:
            (FamixSTONExport new
                entities: aModel entities;
                categories: aModel allCategories;
                associations: self allAssociations;
                yourself)

But I got:

exportOnStream: aStream model: aModel
    (FamixSTONWriter on: aStream) model: aModel.
    (FamixSTONWriter on: aStream)
        writeObject:
            (FamixSTONExport new
                entities: aModel entities;
                categories: aModel allCategories;
                associations: self allAssociations;
                yourself)

The behavior is different and thus it introduced a bug.

Version information (please complete the following information, you can get it from Help->about):

• Version: Pharo 8.0
• Commit info: Pharo-8.0.0+build.1136.sha.a153e04ae4e325509ed78bfe25c9ce560afb24b0 (64 Bit)

[guillep]

The correct fix is to forbid such a refactoring if the temp is used more than once.

https://www.csie.ntu.edu.tw/~r95004/Refactoring_improving_the_design_of_existing_code.pdf

[jecisc]

The temp is assigned only once here.

[guillep]

I meant used. Sorry. Otherwise the refactoring is invalid and cannot be done.

[guillep]

And I'm again mistaken. The refactoring definition does indeed say "assignments".

Using a cascade is only possible if given a sequence of statements, the temp is receiver in all of them. As soon as the temp is used as argument this breaks.

And All inline temps in the world seem to do what Pharo does. Why doing it differently?

[Lin777]

Hi @jecisc , @guillep

In fact the behavior of inline temps is the same as other IDEs such as IntelliJ IDEA

Example: https://www.jetbrains.com/help/idea/inline.html#inline_variable

Before:

public void method() {
    int number = anotherClass.intValue();
    int b = a + number;
}

After:

public void method() {
    int b = a + anotherClass.intValue();
}

Therefore I propose to close this issue, and maybe open a new issue to create a refactoring "Convert to cascade".

IntelliJ IDEA Help

Inline | IntelliJ IDEA

[guillep]

public void method() {
    int number = anotherClass.intValue();
    int b = a + number;
}

Woo, this is from long ago. I think the original question was about what happens when that variable is used many times. Something like:

public void method() {
    int number = b.intValue();
    int b = a + number;
    int c = number / 2;
 }

And you decide to inline the temporary number.

I think what the refactoring is doing right now is wrong, as in "it is not a refactoring because it can break my code". That's why I proposed that at least the refactoring should tell the user this is not possible (and why!).

[Lin777]

Oh I understand. So we should add some rules to decide when this refactoring is applicable. Thanks for the clarification @guillep

[jecisc]

Or if the temp is the receiver of all statements, then inline as a cascade would be the best if it's not too much work :) But forbidding the refactor is still better than the current state.