Bug 10687 - Some Auto-Fix visibility for methods which are nice to have
Summary: Some Auto-Fix visibility for methods which are nice to have
Alias: None
Product: Xamarin Studio
Classification: Desktop
Component: C# Binding ()
Version: 4.0
Hardware: PC Windows
: Normal normal
Target Milestone: ---
Assignee: Mike Krüger
Depends on:
Reported: 2013-02-26 07:55 UTC by Ciprian Khlud
Modified: 2013-02-27 09:42 UTC (History)
1 user (show)

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

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:

Description Ciprian Khlud 2013-02-26 07:55:09 UTC
Description of Problem:
Auto-Fix should hint some fixes:

Steps to reproduce the problem:
1. Create a console application
2. Paste this code as starting point:
using System;

namespace TestConsole
    class MainClass
 		public bool AreAllNumbersPrime (List<int> items)
 			bool result = true;

 			foreach (var item in items) {
				if (item < 4)
 					result = false;
				for (var i = 3; i < item / 2; i += 2)
					 if (item % i == 0)
 					result = false;

                        Console.WriteLine(result?"All numers are prime":"Some numbers are not prime");
			return result;

		public static void Main (string[] args)
			var items = new List<int>{2,3,5,7,11,13,17,19,23,29,37};
			var main = new MainClass ();
			var result = main.AreAllNumbersPrime (items);

3. Go to specific use-cases, that are described bellow
4. Press Alt+Enter to show quick fixes
5. Select the first option

Use cases:
A) Step3. - set the cursor to the word: args for line: public static void Main (string[] args)
Expected result:
 If args is never used, the default thing I would expect to propose is to remove parameter all-together (and all references to the method to remove the call code at that position). 
Actual result:
"Create overload without parameter" which still requires the user to replace in all his code the redundant/unused parameters

B) Step3. - set the cursor to the word: public; for line: public bool AreAllNumbersPrime (IList<int> items)
Expected result:
 As the function is used in the same class (sometimes it can be used in derived class), the Auto-Fix may hint:
- "Make it private" (if the method is called just by this class)
- "Make it protected" (if the method is called by the same class or any class that inherits it)
Actual result:
Nothing appears

C) Step3. - set the cursor to the word: bool; for line: public bool AreAllNumbersPrime (IList<int> items)
Expected result:
 As the function result is not used anywhere
- "Return void" and all returns from the file are replaced with return; If the returns are not for simple variables. The methods are moved before the return. If it would be a method: return DisplayState(); the call will be changed internally to: DisplayState(); return; simple variables are stripped out.

Actual result:
Nothing appears

D) Step3 - set the cursor to the word: AreAllNumbersPrime; for line: public bool AreAllNumbersPrime (IList<int> items)
Expected result:
Function AreAllNumbersPrime does not access This pointer either via fields call or via method or property call.
- "Make it static": would add static in the function definition

Actual result:
Nothing appears

E) Step3 - set the cursor to the word: IList; for line: public bool AreAllNumbersPrime (List<int> items)
Expected result:
 It would be great to notice that is used just IEnumerable part of IList, so it can be changed to IEnumerable<int> (foreach is using the IEnumerable interface). Making your code "IEnumerable aware" makes it very easily later to be mixed with Linq (Linq generates IEnumerable by default)
- - "Convert to IEnumerable": all variables inside AreAllNumbersPrime are not using this pointer, so make it static would make sense (static code gives some warranties in code too, so is better to have as much code as possible as static).

E) Part 2: it may sometimes be nice to propose that it can use the base class in a specific method. But this is a super-set Use-caseE. So if a specific method uses for example a List<...> and the user uses IEnumerable and Count property; to be suggested IList<int> (in the upper code, Count is not used).

Actual result:
Nothing appears
Comment 1 Ciprian Khlud 2013-02-26 08:55:31 UTC
Just to make an extra note:
This line has a small bug in my code:
			var result = main.AreAllNumbersPrime (items);
It should be read in original code like this:
main.AreAllNumbersPrime (items); //no "var result = "

Anyway, the suggested fix: "remove redundant assignment" works bad: 
- as result is not used anymore: result should be removed, so: the function will match the use-case D presented in the bug description

Use case F:
select result from: var result = main.AreAllNumbersPrime (items);
Expected result:
"Remove unused assigned variable" 

			var result = main.AreAllNumbersPrime (items);
and the result should be:
main.AreAllNumbersPrime (items);

Actual result:
"Remove redundant assignment":
and code becomes:
var result;
//it should be maybe bool result!?
Comment 2 Mike Krüger 2013-02-26 11:34:15 UTC
A) It's on the todo list for the refactoring system atm it's only in-file refactoring. I try to push for the source analysis features and that's something we'll add then :)

B) A bit too slow to have that on the keyboard move event since that would require to analyze the whole assembly - and in case of a library it may even be whished that way. Therefore I don't think we'll add that.

C) the same as B) - atm we can't do a whole file analyzation :/

D) Is a nice thing - I think we'll add that.

E) We've such an issue provider but it seems not to be working in that case. I'll try look at it.

F) is bug - I'll look at it.
Comment 3 Ciprian Khlud 2013-02-27 01:33:18 UTC
Just for case D), if the code can decide that a method is static, the actual "renaming" would imply that all usages of the method need to be changed from:
varInstance.MethodName(); to: ClassName.MethodName(); so if you said that the global refactor is not working/limited, maybe this could be postponed too.
Comment 4 Mike Krüger 2013-02-27 07:48:06 UTC
F) shouldn't happen anymore.
Comment 5 Mike Krüger 2013-02-27 09:42:27 UTC
ok closing this bug because you've created single bugs (thanks for that, makes life easier :))