Bug 10743 - Auto-Fix regarding control flow: Simplify branch in loops and functions
Summary: Auto-Fix regarding control flow: Simplify branch in loops and functions
Status: RESOLVED FIXED
Alias: None
Product: Xamarin Studio
Classification: Desktop
Component: C# Binding ()
Version: 4.0
Hardware: PC Windows
: Normal enhancement
Target Milestone: ---
Assignee: Mike Krüger
URL:
Depends on:
Blocks:
 
Reported: 2013-02-27 14:29 UTC by Ciprian Khlud
Modified: 2013-03-26 06:22 UTC (History)
1 user (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Code + Tests (incomplete, it needs review + reformatting) (3.03 KB, application/zip)
2013-03-24 14:04 UTC, Ciprian Khlud
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Ciprian Khlud 2013-02-27 14:29:07 UTC
There are two constructions (very similar) that can be simplified with an equivalent code: (they are inside loops and root of function body
Code:
while|for|foreach(...){
  [instr1; ...; instr n-1; ]
  $if(condition) //if is the last instruction of the looping structure
  { if_block_content; }
}

can be rewritten as:
while|for|foreach(...){
  [instr1; ...; instr n-1; ]
  if(!condition) //if is the last instruction of the looping structure
     continue;
   if_block_content; 
}

In void functions/set property
void functionName| return_type propertyName { set 
{ //start
  [instr1; ...; instr n-1; ]
  $if(condition) //if is the last instruction of the body of the code
  { if_block_content; }
}

can be rewritten as:
void functionName| return_type propertyName { set 
{ //start
  [instr1; ...; instr n-1; ]
  if(condition) //if is the last instruction of the body of the code
     continue;
   if_block_content; 
}

In my experience, this if(s) are the first instruction also in the loops/method bodies. so the code looks like:
void Method()
{
   if(a!=null)
   { 
     foreach(var item in a.Items)
     {
       if(item!=null)
       {
         DisplayItem(item);
         localList.Add(item);
       }
      }
}

After applying both simplifications the code will become:
void Method()
{
   if(a==null)
     return;
   foreach(var item in a.Items)
   {
      if(item ==null) continue;
       
      DisplayItem(item);
      localList.Add(item);
   }
}

I found this issue in NRefactory in 60 cases like: IdStringProvider.AppendTypeParameters() or in CecilLoader class (15 times), DefaultResolvedTypeDefinition(7 times) .

I lobby so much for it, as I found that in real life these code simplifications do improve the code flow as they reduce a lot the branching
Comment 1 Ciprian Khlud 2013-02-27 14:37:08 UTC
I made a mistake, the body of condition has to be rewritten as if(!condition) return;
so text becomes:

can be rewritten as:
(...)
  if(!condition) //if is the last instruction of the body of the code
     return;
   if_block_content; 
}
Comment 2 Ciprian Khlud 2013-02-27 14:41:35 UTC
In NRefactory Git project (not only NRefactory assembly), there are more than 450 cases when this simplification would apply.
Comment 3 Ciprian Khlud 2013-03-24 14:04:02 UTC
Created attachment 3690 [details]
Code + Tests (incomplete, it needs review + reformatting)

Here is a .zip file with patched sources and tests.
Added a basic implementation. ”It works on my machine” (TM) but still I was not able at all to remove the block definition and replace with its instructions.
Comment 4 Mike Krüger 2013-03-25 01:45:21 UTC
Just make an nrefactory pull request next time - it's easier :)
Comment 7 Mike Krüger 2013-03-26 02:25:30 UTC
Good work for your first actions :). I merged the actions in NRefactory:

https://github.com/icsharpcode/NRefactory/commit/116d6036bfe1d09e51f8d5a53101d10f0c6d60f7

Some things I changed:

+ Added standard headers (btw. monodevelop supports auto adding headers - it's in the source code options. Just choose MIT-X11
+ Reworked the unit tests - seems that you had very old tests as example :)
+ Removed the obsolete block that could be a left over from an if body (you tests have shown that). Actions should do stuff like that :)

For stuff that doesn't work just make a unit test for it that get's ignored. It's better than a TODO comment.
It's easier for other people to see what's missing.

Some more unit tests (maybe a bit more complex) would've been nice.

Thanks