Code Action Idea: Reorder If/Else Branches about js-assistant HOT 12 CLOSED

Great idea! I'm thinking about introducing a "move" class of refactorings for reordering and moving parts of the code without changing it. This could fall into that category.

from js-assistant.

My main concern is the ambiguity of what up/down means for the space in between two elements. I'll think about it

Ask some users what they would expect to happen with move up/down when the cursor is on the else branch.

Then ask them what should happen when invoking swap here:

if (cond1) {
    body1
} else if (cond2) {
    body2
} el|se if (cond3) {
    body3
} else if (cond4) {
    body4
} else if (cond5) {
    body5
}

And then ask them which question they felt more comfortable to answer.

from js-assistant.

I changed the refactoring to move up / move down:

Available in v1.132.0

from js-assistant.

I've introduced a "swap if-else-if branches" refactoring in P42 v1.131.0:

Thanks again for hte refactoring idea!

from js-assistant.

Nice work!

What does swap branches do exactly though?

I think a reactoring "move branch up" and "move branch down" would be more intuitive...
Especially when integrated into a larger group of "move up / down" code actions that are all bound to the same keybinding.

from js-assistant.

Swap exchanges the 2 adjacent if-else-if branches.

I was originally thinking the same (having move up/down). Now I think there are 3 cases depending on where you try to activate the refactoring:

• on the first "element": move down
• in between elements: swap
• on the second "element": move up

(this could apply to any move up/down refactoring, e.g. to function parameters or to class methods)

Here I've only implemented swap for now, because it can be activated on the "else if" (since this was the activation range that you recommended). Do you think other activation ranges would be helpful?

Regarding keybindings, there is now a "refactor.move.swap.*" code action id prefix that could in the future be used for different swap refactorings (e.g. between function arguments) - other prefixes could be "refactor.move.up.*" and "refactor.move.down.*" in the future.

from js-assistant.

in between elements: swap

This I don't understand. Why not move down/up here too to unify the experience?

Start:

if (cond1) {
    body1
} el|se if (cond2) {
    body2
} else if (cond3) {
    body3
}

Start --- Move up --->

|if (cond2) {
    body2
} else if (cond1) {
    body1
} else if (cond3) {
    body3
}

Start --- Move down -->

if (cond1) {
    body1
} else if (cond3) {
    body3
} el|se if (cond2) {
    body2
}

from js-assistant.

My current interpretation is that when you are on the "else", you are not on an if element, so it is not clear what you would move "down". But this might be too technical of a notion (based on the AST). I'll think a bit more about this - I agree that having an intuitive and unified system is desirable.

Here is my current thinking:

Start 1:

|if (cond1) {
    body1
} else if (cond2) {
    body2
} else if (cond3) {
    body3
}

Start 1 --- Move down --->

if (cond2) {
    body2
} else |if (cond1) {
    body1
} else if (cond3) {
    body3
}

Start 2:

if (cond1) {
    body1
} else |if (cond2) {
    body2
} else if (cond3) {
    body3
}

Start 2 --- Move up -->

|if (cond2) {
    body2
} else if (cond1) {
    body1
} else if (cond3) {
    body3
}

Start 3:

if (cond1) {
    body1
} el|se if (cond2) {
    body2
} else if (cond3) {
    body3
}

Start 3 --- Swap -->

if (cond2) {
    body2
} el|se if (cond1) {
    body1
} else if (cond3) {
    body3
}

from js-assistant.

Here is my current thinking:

I wouldn't distinguish between el|se if and else i|f here.

I wonder what swap would do on the last else in start3?
Personally, I find the description "Move branch 3" up easier than "Swap branch 3 (with branch 2 or branch 1?)".

from js-assistant.

I wonder what swap would do on the last else in start3? Personally, I find the description "Move branch 3" up easier than "Swap branch 3 (with branch 2 or branch 1?)".

Swap always swaps the adjacent branches. This is the current implementation:

Start 4:

if (cond1) {
    body1
} else if (cond2) {
    body2
} el|se if (cond3) {
    body3
}

Start 4 --- Swap -->

if (cond1) {
    body1
} else if (cond3) {
    body3
} el|se if (cond2) {
    body2
}

One challenge is the action label - to distinguish the branches in the action label, I could include e.g. the condition, but this will lead to issues with long conditions.

from js-assistant.

I think swap is also a more mentally complex operation: "Swap" exchanges the upper element with the lower element - so mentally, you have to think about both elements.

However, "move up" focuses only on the current element.

Effectively, they would do the same though.

from js-assistant.

Yeah, I agree that's a good point. My main concern is the ambiguity of what up/down means for the space in between two elements. I'll think about it - for e.g. statements up/down is the best option for sure, and for if/else I could add it as well with different activation zones. I'll re-open this issue to keep track of it.

from js-assistant.