| Author |
 Topic  |
|
|
jay.carlton
Ketchup Master
USA
65 Posts |
 Posted - Feb 20 2014 : 10:05:33 AM
|
Hi,
Often I have a conditional block that becomes unconditional, and I need to remove the line with the if statement, the opening and closing braces, and unindent things. All these steps are quick, except when the body is lengthy or deeply nested. So the refactoring would be something "Remove Condition (keep body)".
A frequent scenario for me is refactoring a deeply nested function with only one return statement and many nested ifs where I want to have several error handling branches with early return statements first and lower nesting overall.
Also, "Replace Nested Conditional with single Condition" might be nice.
Say you have
if (bCondition1)
{
if (bCondition2)
{
if (! bCondition3)
{
// code that does the work
}
}
}
If I could select only the innermost body and say Simplify Nested Conditions, then I could have
if (bCondition1 && bCondition2 && (!bCondition3))
{
// code that does the work
}
This one would only make sense for pure nested ifs, with no elses or following code at any level.
Thanks,
Jay |
|
|
feline
Whole Tomato Software
United Kingdom
19003 Posts |
Posted - Feb 26 2014 : 11:11:15 PM
|
Removing the if statement, or another surrounding block, and fixing the indent makes sense and is an interesting idea. I have put in a feature request for this:
case=80541
I am not so sure about combining the if statements like this. Is this something you often need or want to do? It looks like the sort of change that could produce a lot of edge cases, or simply if statements full of brackets that are hard to read quite quickly and easily. |
zen is the art of being at one with the two'ness |
 |
|
|
jay.carlton
Ketchup Master
USA
65 Posts |
Posted - Feb 26 2014 : 11:51:43 PM
|
| Thanks. The nest if operation is not something all that common, but I'm always looking for ways to work at a higher level of abstraction. Just thinking out loud I guess. |
|
|
|
feline
Whole Tomato Software
United Kingdom
19003 Posts |
Posted - Feb 27 2014 : 10:01:34 AM
|
| To be honest I see the appeal of the idea of merging if statements like this. I am also aware from my own experience of how hard to read if statements get once you get past 2 or 3 clauses, and suspect this could easily end up making things less clear and helpful, rather than more clear and helpful. |
zen is the art of being at one with the two'ness |
|
|
|
jay.carlton
Ketchup Master
USA
65 Posts |
Posted - Apr 14 2014 : 12:36:07 PM
|
Any toolbox with sharp tools in it has the potential for abuse. I will grant that this kind of operation would be much safer if combined with its inverse, i.e. splitting complex predicates into separate nested if and else clauses. Once I had confidence in the accuracy and reversibility of the operation, I could switch back and forth between logically equivalent representations and find the most readable one.
There are some other common variations on this theme:
* switch if and else branches, inverting the condition so the logic is the same
* Identify candidates for sub-functions based on cohesion within branches
* suggest boolean functions based on frequently occurring or complex predicates (or clauses within them)
* automatically run through several combinations that give the same behavior and select the least complex one, based on a tunable set of metrics
It could be a small step into some interesting territory, where you look at the set of possible refactorings to achieve a goal and pick the best one. Right now, most of the refactorings offered deal with structure rather than logic, and there is probably some low hanging fruit there. |
|
|
|
feline
Whole Tomato Software
United Kingdom
19003 Posts |
Posted - Apr 15 2014 : 10:39:30 PM
|
We are looking into swapping if statements around:
case=27848
Obviously something we need to do carefully, since it needs to be reliable, even for complex logic statements.
I am not sure what you are getting at with "Identify candidates for sub-functions based on cohesion within branches". Having VA suggest blocks that you might want to extract sounds interesting at first, but the more I think about this, the more I have my doubts. Simply identifying a block that can be easily extracted does not tell us anything about how sensible this would be, or if this given block of code makes sense as a function. Or am I missing something here? |
zen is the art of being at one with the two'ness |
|
|
|
jay.carlton
Ketchup Master
USA
65 Posts |
Posted - Apr 15 2014 : 11:04:00 PM
|
There are metrics that look at cohesion of variables to an extent, and there might be analyses you could do to determine strategies for cutting up really large functions, assuming you could identify inputs and outputs of such chunks.
For instance, if you had a block of code, either as part of an if statement or just a pair of braces without any if or while or for on it, and if this scope made changes to various things but you could determine that only one value in it "mattered" to any code after the block, then this block might be identifiable as an extractable function candidate, especially if there were no side effects (i.e. nothing that needed to be passed in by non-const reference or pointer). Then VA could highlight or draw a box around that chunk of code and offer to extract it as a separate function.
Some other clues might help inform the heuristics here. One big one is whether the same piece of code appears in multiple places in the file. (Bonus points if it's the same except for the names of local variables declared within the scope of the block). If you have three or four blank lines separated by white space, sometimes those stanzas make good helper functions, since the original function author thought of them as separate steps. [There's another feature request case somewhere about auto-selecting multiline blocks of code, but I don't remember where.]
Another thing I would say regarding the sensibility is that sometimes I do refactorings of really large functions in multiple passes, and if I can make medium sized functions out of huge ones, and break those down further into small but awkward functions, I'm then in a better position to apply some sanity and elegance to the solution. Often times I'll even give these ugly half-refactored functions really long names that describe exactly what they do but don't really provide any abstraction (like CheckLineThreeOfTheSecondFileForAPercentSignIfItsTheFirstTuesdayOfTheMonth()), and then that's a solid code smell telling me that it needs a couple further rounds of manual refactoring, and that it was probably formed by an extraction. All that to say that the place for automatic detection of logically extractable chunks could be useful in speeding up the early stages of this kind of refactoring effort, and could potentially improve the accuracy and usability of Extract Method as well.
But anyway, easily rearranging conditionals and logical predicates would be an awesome way to focus on logic rather than text editing, which is what refactoring tools are all about.
Thanks. |
|
|
|
feline
Whole Tomato Software
United Kingdom
19003 Posts |
Posted - Apr 16 2014 : 7:39:17 PM
|
Do you know anything about metrics for looking at cohesion of variables? This is an interesting line of thought, and its not really something we have ever tried to do, to the best of my knowledge.
We are considering having Extract Method look for the extracted block, to see if it has been reused, and updating those sites as well:
case=4313
your bonus point about local variables with different names is interesting, and I have added a note to the case about this. I am not sure we would actually try and do that, but it is worth considering.
You are right, we are considering a feature to expand / shrink the selection one "step" at a time:
case=28489 |
zen is the art of being at one with the two'ness |
|
|
|
jay.carlton
Ketchup Master
USA
65 Posts |
Posted - Apr 16 2014 : 7:46:08 PM
|
I'm not an expert, but I know NDepend and CppDepend have some metrics like that, and most of those are from open literature. I think it's some kind of covariance of the distance between pairs of symbols within a type or assembly (but I imagine you could apply it to any chunk of code.
edited by admin May 7 2014: removed link to ndepend.com because destination page contains malware.
|
|
|
| |
Topic |
|
Remove matching brace when removing if/for/while
