-
Notifications
You must be signed in to change notification settings - Fork 1k
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal] Labeled loops like in Java #1597
Comments
@AustinBryan Related #869 |
Also relevant: dotnet/roslyn#5883 |
I like labeled break much better. Also would |
Yes, it should. I just tested it out on Java and it works for that so it should for C#. |
In the last three years I needed only once a |
@AustinBryan I think that your second case was rejected, you can check the post @Joe4evr linked. |
So |
@Neme12 It's clearer about what's going on. More symmetrical with |
@Neme12 I mean yes, except it reflects more on what we want to do. In this example, what I want to do is |
I disagree. the cleanest way is to exit the function, avoiding
If you end up with code that needs a |
I kind of agree - in code reviews, I usually see nested loops as a code smell indicating a need to break out another method. |
@theunrepentantg
Wouldn't that be an extreme? I mean, indeed, sometimes it's the case but saying that you usually tend to find nested loops a code smell when a code smell should indicate that there's a problem with the design not with how the code is structured but more importantly this statement shouldn't be taken as rule of thumb because what's applicable to one codebase might not be applicable to another, one might be more imperative in nature whereas the other can be more declarative and so it really depends. Refactoring too much can also lead to readability and maintainability issues. |
I totally agree - readability, maintainability - and all the 'ilities (scalability, supportability, etc) are very important. Nonetheless, I stand by the statement: Usually nested loops are a design smell.
I don't understand this - how can well-designed code be structured poorly? Design covers all scales - from the macro architecture (components, services, etc) to the micro (algorithm choice, method signatures, local variable naming, etc). |
@theunrepentantgeek I guess that this is where we disagree, you seems to think that nested loops is an indication of a code smell whereas I think that it might be the case but you don't always need to refactor nested loops especially in this case where the language can be improved a bit and make the You can structure the code in many different ways, you can structure it through a series of imperative statements, you can use a more procedural approach, you can use a more declarative approach and apply all kinds of functional programming techniques and yet the design can be broken so just because you refactored something into its own method and you gave it a name and it looks good doesn't mean you improved the design, in fact, it might mean the opposite but really context is important here and in this case the kind of codebase you work with should be considered before going out and tell people what they should or shouldn't do. Now, some people would consider their codebase state of the art because it's refactored and follow awesome set of principles and whatnot only to find out that it's actually poorly designed. |
Such code runs counter to the "code should describe the intent; not the mechanism" principle of easy-to-reason (or "reasonable") code. So if I write code like: var found = false;
for (var i = 0; i < range1; i++)
for (var j = 0; j < range2; j++)
if (i * j == 10)
{
found = true;
goto outer;
}
outer: {} I'm purely expressing the mechanism. The code is highly unreasonable. Therefore the next thing I'm likely to do is add a comment to explain its intent: // loop through the two ranges and report if they contain factors of 10
var found = false;
for (var i = 0; i < 5; i++)
for (var j = 0; j < 3; j++)
if (i * j == 10)
{
found = true;
goto outer;
}
outer: {} Now I have the intent, but I still have the mechanism there too. It's a mess. I'm not sure If it's a code smell, but it certainly is ugly and difficult to read. What I want is the intent only: var found = RangesContainFactorsOfTen(5, 3); The code is now trivially easy to reason. And when I want to understand the mechanism, I head off to that method: private bool RangesContainFactorsOfTen(int range1, int range2)
{
for (var i = 0; i < range1; i++)
for (var j = 0; j < range2; j++)
if (i * j == 10)
return true;
return false;
} So I've achieved code that is far easy to reason and read, I've removed the comment and I've avoided the need for a C# is already complicated enough with a growing number of features with every release. Those new features should be focused on the "pit of success", ie making it easier to write good code. Labelled breaks are the opposite: they just offer another way, in addition to the existing one, of writing bad code. So they they have no place in future versions of the language in my view. |
@DavidArno First of all, I agree, if there's a value in refactoring, you should do it but my point was that if you have a method that is self-explanatory you wouldn't want to refactor it further just because now you need a
I didn't say that now you should use
I really don't understand why would you consider label breaks as "bad code" when at rare times it can actually improve the code. |
@DavidArno - In your examples you have no further processing after the label I recall many occasions where an organization had a coding standard "only one entry point and one exit point" and your example is reminiscent of (though not the same as) this kind of rule. I recall the pain of having to follow that rule even in the parameter validation (the language had no exceptions) with mounting levels of nested if/then/else - ultimately greatly cluttering the code. Being able to exit a loop at any arbitrary depth is in fact a powerful simplifying mechanism, its intent is obvious "where 100% done looping here" and its effect is clear "continue at the statement following the end of the exited loop". Granted there are probably many cases where it can be avoided with possibly simpler design but I'm sure there are cases too were being unable to break like this forces the logic to fiddle around unnecessarily. |
@Korporal you seem to miss what @DavidArno said about extracting methods for each nested loop. You can rewrite that code so that is easier to read, refactor and understand. Period. Replacing goto with loop lables doesn't help, it just moves the problem. Refactoring and extracting a method for every loop is the way to go. There are tools for this, but you should be able to do it on your own anyways. It is also less error-prone that nested loops in their raw form, and easier to refactor. |
@Mafii - By your reasoning then the existing keywords The existing break keyword can be read as: |
"My" examples are simply @AustinBryan's examples, restructured to avoid the need for breaking out of a nested loop. I don't think Austin's examples are contrived at all.
It's the absolute opposite. In fact such refactoring only really works if that silly rule is ignored.
You are missing the point. Folk tend to reach for It isn't a case of avoiding A real world example. I went looking for somewhere where I'd used public static Option<T> TrySingle<T>(this IEnumerable<T> collection, Func<T, bool> predicate)
{
if (predicate == null) throw new ArgumentNullException(nameof(predicate));
var result = Option<T>.None();
var count = 0;
if (collection == null) return result;
foreach (var element in collection)
{
if (!predicate(element)) continue;
if (++count > 1) break;
result = Option<T>.Some(element);
}
return count == 1 ? result : Option<T>.None();
} This is an example of what I was talking about before. I test public static Option<T> TrySingle<T>(this IEnumerable<T> collection, Func<T, bool> predicate)
{
if (predicate == null) throw new ArgumentNullException(nameof(predicate));
var result = Option<T>.None();
var count = 0;
if (collection == null) return result;
foreach (var element in collection)
{
if (!predicate(element)) continue;
if (++count > 1) return Option<T>.None();
result = Option<T>.Some(element);
}
return result;
} A tiny change, yet I've reduced the routes through the code and so reduced it's complexity and thus the likelihood of a bug. |
Considering all the little enhancements they've been introducing to C# (like returning tuples) I think labeled loops with break/continue labels would be a straightforward, useful feature that should be no problem. I'm not sure why this hasn't been done yet. |
An LDT member already said this on the matter:
Now, can we please close this thread and move on? |
Yeah I noticed that. It's ridiculous. Is that how C# gets developed, one arrogant guy dictating everything with no justifications? |
The justification is obvious: you can already do this using |
Yes. The LDM decides what goes in the language. It's not a free-for-all where people just propose things and they just get added :) Note: if you want, you can always fork the language and do whatever you want with it. However, MS controls the direction and decisions around C#-proper. |
@DavidArno That's not the same, the goto label points to the end of the loop which is much less intuative / easy to read than pointing to the start of the loop. You also can't do a break with that goto; it has to be the equivalent of a continue. You'd need 2 goto labels to choose between a break or a continue. |
@Joe4evr - But what about the fact that this is (probably) quite easy to add to the language, involves no new radical concepts and potentially rather useful in general, this is what I would call "low hanging fruit" apart from the disagreements here (which reflect tastes) adding this should be pretty straightforward and carry pretty low risks. |
I, sincerely, disagree entirely. You intend to jump out of the loop, so pointing to the start of the loop is entirely less intuitive. There are already good, clean ways to break out of nested loops that you can use:
var found = false;
for (var i = 0; i < range1 && !found; i++)
for (var j = 0; j < range2 && !found; j++)
if (i * j == 10)
{
found = true;
}
if (found)
Console.WriteLine("We found a factor of ten"); Asking for something that actively makes code look worse should not be an option. |
I'm participating in this discussion because I have felt strongly over the years of using |
I'm also really tempted to fork the repo, implement the change and submit a pull request and be done with this rather simple change. |
I would def recommend doing that! That's a core value prop of an open source C#/Vb compiler :) |
I'm listening, and this isn't the first time I've tried going down this path myself. What happens when there are three reasons for exiting or continuing the outer loop? |
I def got that! The purpose in my response was to point out that i felt there was a solution that actually addressed your reasons. It felt like you didn't like the solution for reasons unrelated to the solution itself, but because it was through analyzers. I was pointing out then that that argument then seemed to indicate that analyzers themselves weren't really ever viable if used to restrict the available surface area of C#. Did i misinterpret things? If so, again, apologies.
What would you do with a labeled_loop in that case? Would you have three labels for the loop? Or just one? How would you indicate the reasons for exiting? |
@CyrusNajmabadi My argument was that there is an impedence mismatch, and the need for an analyzer was the smallest part of that argument. I'm not saying analyzers are bad. For example, I'm writing an analyzer to generate and maintain With methods. The need for such an analyzer is one of the reasons I'd ask for a withers feature, but by no means the only reason. That reason can't stand in isolation, but it's a real enough reason when taken with the rest.
I wouldn't. I'd name the loop. The suggestion to indicate the reason for exiting came from @HaloFour and I am responding to him here, asking the same question you just asked. |
Why is this relevant? We do not currently indicate the "reasons" for doing things in our code other than the code itself perhaps an |
Ok... so if you'd name the loop... why can you not just label the appropriate jump points if you were to use 'gotos'. I feel like i'm not getting your argument here. :) |
Well, for starters, I wouldn't, because that's awful. I don't write Java like that. I don't allow my devs to write Java like that. But like anything else that can be named I would choose names that fit the logical reason for wanting to do something. |
Understood. Sorry for trivializing your argument hte away i did. |
Jumping back a bit. I can see the argument for why labeled-loops seem to be desirable. They have the following pros (depending on your perspective).
Overall, these are a cogent and clear argument for why such a feature at least "makes sense" in isolation. And, frankly, i'd likely be swayed by this for C# v1 to have this instead of having hte arbitrary-goto feature. Clearly the above (and likely other stuff i missed) is sufficient for some people to really want this even for a v8+ release. I can accept and respect that. -- At this point it really comes down to: Is there an LDM member that thinks the above is sufficient? Or is the alternative we have today (gotos + optional-analyzers) reasonably sufficient (and cost effective) to just stick with the status-quo. My personal opinion is "the status quo is fine". However, not everyone may share htat opinion. We'll just have to see if someone wants to champion this. |
@CyrusNajmabadi All good, thanks for listening!
The latter is better documentation of the loop declaration and feels right. The former feels like a hack, is harder to set up and read, and doesn't contain richer information. Possibly less rich. |
Labels are just a means to an end a way of identifying the loop to which the
or
I'm not advocating that of course but do want to point out that a label is not the only way to provide the information to the |
It's very interesting how different people see these things. I already view a loop as incredibly low-level and imperative. To me your To each their own. |
Huh. It's probably also worth pointing out that I'd only find myself doing this if I was inlining for very specific hot paths. Otherwise I'd use |
That's my problem with it. It documents the loop declaration, not the reasons for wanting to leave the loop. If you have two reasons for wanting to |
@HaloFour That's consistent with how I use |
IN that case, i think it's totally reasonable to just do the same with "goto" :) At this point, i think the arguments have been exhausted. I think i totally get your position @jnm2 . it's consistent and not incorrect in any fashion. It will be up to an LDM member to see if they want to champion.
Seems totally clear to me :D |
Please note that when we talk about cost, it's not just "how hard is this to implement in the compiler"?
You're welcome to do that for you own use but please know that if you plan to submit a PR to Roslyn, you likely won't even be allowed to keep it open or have a feature branch unless this feature is championed. If you find it fun to experiment with this or just implement in case it would get considered in the future, that's awesome but don't be disappointed if that doesn't happen. |
I certainly would be interested in making such a change but there's clearly a ramp up time and we know the team already has the expertise for this. What I was trying to convey is that it is probably within my abilities despite my limited knowledge of roslyn, unlike some other changes. I have no idea how the roslyn design ultimately works but one could possibly just implement some form of transformation that transforms this:
into this:
which is why I described this as syntactic sugar earlier. |
Yup. That's how roslyn works already today. Indeed, as IL has no concept of loops at all, all loops in roslyn are ultimately "lowered" into just equivalent forms with appropriate "gotos". So you could certainly implement this feature in such a manner. |
I took some time to create the spec for this feature here, if that helps |
This feature is not championed by any member of the LDM. Until that happens, there will be no progress here. |
There's a use case I've come across a few times now that's not mentioned here: needing to use Since pattern matching was introduced to the language and keeps improving I find myself using them more and more, but a number of times I've found myself having to write this annoying kludge: var @break = false;
foreach (var c in input)
{
switch (c)
{
case 'a':
// snip
break;
case 'b':
// snip
break;
case > '0' and < '9':
// snip
break;
case 'c':
// What I really want to do here is break out of the loop, not the switch statement
if (anyDigits)
{
@break = true;
}
break;
default: @break = true; break;
}
// Now I have to check my boolean to break myself
if (@break) break;
} It's possible this particular usecase could get its own feature, such as |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Currently the cleanest way to break out of a nested loop is this:
If all we wanted to do was break out of the loops, we have to do
outer: {}
, which is sort've clunky. What we could do instead is use labeled loops like Java, so we can do this:And instead of:
We can just do:
The alternative is cleaner, more intuitive, and simpler.
I saw another post where they suggest doing
break 1;
to break the first loop, but that doesn't refactor well if the number of loops changes, you have to carefully make sure the numbers are correct.The text was updated successfully, but these errors were encountered: