Community
Participate
Working Groups
I have found that it is very common in poorly written code to find very long if statements that can be safely "inverted" in so that the the code is easier to understand and maintain. For example the wrong way is this: public void foo() { if (x) { ...do a bunch of stuff... return; } } When the elided part is long the if statements add considerably to the cognitive effort of holding this code in short term memory. I always refactor such code as such: public void foo() { if (!x) return; ...do a bunch of stuff... } Now you don't have to hold all of that code to be within that if statement in your head, but the code will execute identically. Please add an automated refactoring for this feature.
We have an invert if-else-statement quick assist, but it currently only works if there is an else block
I tested that, it does not do what I'm looking for. I want to convert if (x) ...do stuff.. return to if (!x) return ...do stuff... The quickfix just swaps the if and the else. It does not improve the readability of the code as this does. A more extreme example: foo() { if (x) { if (y) { if (z) { .. do stuff... } } } } is much better as foo() { if (!x) return; if (!y) return; if (!z) return; ..do stuff.. } The first one is a very poor coding style but unfortunately common. As long as the if exits at the end and there is no other code to execute, doing this refactoring is completely safe and it could be automated. (In reply to comment #1) > We have an invert if-else-statement quick assist, but it currently only works > if there is an else block >
We can probably 1) provide 'Invert 'if' statement' quick assist on If statements missing an else block. if (x) {...do stuff..} => if (!x) {} else { ...do stuff..} and/or 2) provide a 'Return if condition is false' quick assist if (x) {...do stuff..} => if (!x) return ...do stuff..
(In reply to comment #3) > We can probably > 1) provide 'Invert 'if' statement' quick assist on If statements missing an > else block. > if (x) {...do stuff..} => if (!x) {} else { ...do stuff..} I would treat this as a separate feature request. > 2) provide a 'Return if condition is false' quick assist > if (x) {...do stuff..} => if (!x) return ...do stuff.. This is this feature request. I would name it 'Convert to if-return' or maybe 'Convert to early exit' if (x) { code; [ return;] } ==> if (!x) return; code; We should only offer the quick fix if there are no further statements after the if statement, otherwise we produce wrong code.
Created attachment 226020 [details] Patch
(In reply to comment #4) > > 2) provide a 'Return if condition is false' quick assist > > if (x) {...do stuff..} => if (!x) return ...do stuff.. > > This is this feature request. I would name it 'Convert to if-return' or > maybe 'Convert to early exit' Provided the 'Convert to if-return' quick assist via the patch attached above. Following restrictions are applicable: 1. 'if' should be present in a method that returns 'void'. 2. The immediate parent of 'if' should be a Block. 3. At least one statement (other than 'return') should be present in the 'then' statement of 'if'. 4. 'if' should have no further executable statements in the method. 5. 'if' should not be present in a loop. Dani, please review and comment on the behavior and patch.
Created attachment 226181 [details] Test cases Updated the existing test cases and added new ones for all the cases listed above along with some positive test cases.
The patch looks good. Some minor details: 1) The code to detect whether a type is void is twice in the class now. We should add #isVoid. 2) #isLastExecutableStatementInMethod could just work on a Statement. 3) The tests are not 1.7 specific and hence should go into AdvancedQuickAssistTest. 4) The new tests should also assert the number of proposals. 5) The tests you tweaked in QuickFixTest should verify all proposals (I know this isn't the case for all of them right now in master). Please add it for the missing ones and your new proposal. Hint for creating Quick Fix tests: 1. have the JDT test projects in your workspace 2. start a target workspace 3. create an example (must have a warning or error that has Quick Fixes) 4. select the example (full file) 5. Quick Fix > Create quick fix test Hint for test snippets: 1. have the JDT test projects in your workspace 2. start a target workspace 3. write the example code 4. select your example (can be any piece of code) 5. Quick Fix > Wrap in buf.append() (to clipboard)
Created attachment 227137 [details] Patch (In reply to comment #8) > 1) The code to detect whether a type is void is twice in the class now. We > should add #isVoid. > 2) #isLastExecutableStatementInMethod could just work on a Statement. > 3) The tests are not 1.7 specific and hence should go into > AdvancedQuickAssistTest. > 4) The new tests should also assert the number of proposals. Done. > 5) The tests you tweaked in QuickFixTest should verify all proposals (I know > this isn't the case for all of them right now in master). Please add it > for the missing ones and your new proposal. Verifying all the proposals now. Done for the tweaked tests as well as all other tests in AssistQuickFixTest. > Hint for creating Quick Fix tests: > Hint for test snippets: Thanks for these hints Dani. It saved a lot of time.
Created attachment 227138 [details] Patch Updated a test in AssistQuickFixTest.
Created attachment 227175 [details] Patch (In reply to comment #10) Attached the wrong patch above. Please take the correct one.
#isVoid() is not a beauty ;-). I've fixed that before committing. Take a look in 'master'. > Done for the tweaked tests as well as all other tests in AssistQuickFixTest. Thanks! Next time, we should do this in a separate bug, so that a patch only covers the fix for the bug at hand. Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=aa32ffa64427a7e231222cf301563cfbc3ed0a0e
Markus suggested a better name, and I liked it too: Convert to 'if-!-return' Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d82c228f427df19d2243bd6419d2b22e0c7d073b
Verified in I20130313-2000.
