Resharper removes the exit from foreach. What for?
I recently learned about yield
and then created the following test console program:
public static string Customers = "Paul,Fred,Doug,Mark,Josh";
public static string Admins = "Paul,Doug,Mark";
public static void Main()
{
var test = CreateEfficientObject();
Console.WriteLine(test.Admins.FirstOrDefault());
//Note that 'GetAllCustomers' never runs.
}
public static IEnumerable<string> GetAllCustomers()
{
var databaseFetch = Customers.Split(',');
foreach (var s in databaseFetch)
{
yield return s;
}
}
public static IEnumerable<string> GetAllAdmins()
{
var databaseFetch = Admins.Split(',');
foreach (var s in databaseFetch)
{
yield return s;
}
}
static LoginEntitys CreateEfficientObject()
{
var returnObject = new LoginEntitys {};
returnObject.Admins = GetAllAdmins();
returnObject.Customers = GetAllCustomers();
return returnObject;
}
}
public class LoginEntitys
{
public IEnumerable<String> Admins { get; set; }
public IEnumerable<String> Customers { get; set; }
}
But I noticed that Resharper wants to convert my loops foreach
to:
public static IEnumerable<string> GetAllCustomers()
{
var databaseFetch = Customers.Split(',');
return databaseFetch;
}
Why does Resharper want to remove income from this case? It completely changes the functionality as it won't be a lazy load without damage. I can only guess that either
- A) I am using
yield
/ in bad pratice wrong - B) This is Resharper's bug / suggestion that can be simply ignored.
Any insight would be great.
source to share
You are correct that this proposed transformation changes the functionality of the code in a subtle way, preventing it from deferring property evaluations and performing evaluations Split
as early as possible.
Perhaps those who implemented it were well aware that this was a change in functionality and felt it was still a useful suggestion that could be ignored if important semantics were important or if they really didn't understand that the semantics were changed. There is no good way to get to know us, we can only guess. If these semantics are important to your program, then you are correct not to suggest a conversion.
source to share
I think Resharper is a bit dumb here in the sense that its applying the standard "convert foreach to LINQ" conversion without being aware of the context.
It doesn't suggest the same changes for the loop while
:
public static IEnumerable<string> ReadLineFromFile(TextReader fileReader)
{
using (fileReader)
{
string currentLine;
while ((currentLine = fileReader.ReadLine()) != null)
{
yield return currentLine;
}
}
}
My guess is that the next iteration of Resharper that uses Roslyn will be much more contextual.
Thanks @servy for an interesting and refreshing discussion!
source to share
The code in your example does not call the iterator on the IEnumerable
one you return. If you used the result GetAllAdmins()
in a LINQ query, for example, yield
it would be helpful because execution of the expression could resume at each iteration.
I would suggest that Resharper is just asking you to remove unused code.
source to share