-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code Action Idea: Reorder If/Else Branches #28
Comments
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. |
Nice work! What does swap branches do exactly though? I think a reactoring "move branch up" and "move branch down" would be more intuitive... |
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:
(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. |
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
} |
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
} |
I wouldn't distinguish between I wonder what |
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. |
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. |
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. |
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. |
(| cursor)
-- Move Item Up ->
Could use the same shortcut for reordering parameters/members etc.
The text was updated successfully, but these errors were encountered: