Simplify nested loop logic

Question

I am having some trouble trying to simplify some logic that is contained in nested loops

I have provided some pseudo code below, that is the basics of what I'm dealing with. The actual code is more complex and contains multiple nested loop structures.

I'm in charge of maintaining this code, I'm not the original writer of it

Would this be a place that a goto is valid.

while(CONDITION)
{
  while(CONDITITION2)
  {
    if(CONDITION3)
      break CONDITION2

    if(CONDITION4)
      break CONDITION
  }
}

There are many complexities to preforming these code breaks.

I am not allowed to use a goto, but I think it would be an easier solution that what I am currently trying to do. Should I try and justify the use of a goto in this case. thanks

PRASHANT :)

Please check: http://stackoverflow.com/questions/46586/goto-still-considered-harmful — user194076, Feb 23 '11 at 18:30

score 3 · Answer 1 · answered Feb 23 '11 at 18:30

3

Introduce a variable

bool breakOuter = false;
while(CONDITION)
{
  while(CONDITITION2)
  {
    if(CONDITION3)
      break;

    if(CONDITION4)
    {
      breakOuter = true;
      break;
    }
  }
  if(breakOuter)
    break;
}

answered Feb 23 '11 at 18:30

Femaref

60,705
7
138
176

1

I personally disagree with this. All it does is make two jumps instead of one, so an already potentially confusing situation only gets harder to follow (more spaghetti.) With your version, because you're always breaking only one layer, when you really want to break to the outer loop isn't as clear. – corsiKa Feb 23 '11 at 21:18
1

Point is, you can't break two layers in C#. In some cases, this is the only way to achieve double breaking, for example if you can't introduce the predicate in either of the while loops, or if the breaking condition for the outer loop is only accessable in the inner loop. While I agree with you, there are cases where there is no other way. Just make sure to comment it so the next person (or yourself) can understand it later on. – Femaref Feb 23 '11 at 21:42

score 1 · Answer 2 · answered Feb 23 '11 at 18:32

Use a variable for condition 1:

bool condition1 = true;
while (CONDITION && condition1)
{
    while (CONDITION2)
    {
        if (CONDITION3)
            break;
        if (CONDITION4)
        {
            condition1 = false;
            break;
        }
    }
}

score 1 · Answer 3 · answered Feb 23 '11 at 18:32

When Java (not C#, but relevant) made the decision to disallow goto, they found through code profiling that 90% of all gotos were used for loops, and the rest were used for really bad stuff.

Break and continue still exist because they're legitimate, localized gotos that work. Make sure to document where they go and why they do it, but they're still very useful.

You appear to be using them in the most effective way.

Mark Sowul · Accepted Answer · 2011-02-23T21:33:08.770

1

You're setting yourself up for messy and complicated code. Consider LINQ if possible (loops themselves are practically a code smell these days, especially complicated ones), otherwise consider refactoring the inner loop into a separate method and using return;.

Edit: great post by Eric Lippert critiquing the various alternatives for this problem: http://blogs.msdn.com/b/ericlippert/archive/2010/01/11/continuing-to-an-outer-loop.aspx

edited Feb 23 '11 at 21:33

answered Feb 23 '11 at 18:33

Mark Sowul

10,244
1
45
51

What I mean is a signal that maybe there's a better way to do it. Just because it's old doesn't automatically mean that, but, well - take a look at the post I linked to above. – Mark Sowul Feb 23 '11 at 21:32
@Mark Sowul i am of certain of many better way. i am worry of company policy of bug from me from rewrite.. – PRASHANT P Feb 23 '11 at 22:35
That's certainly a valid concern. Then I would say the best way is pulling the inner loop out to its own method (method two in the blog post). – Mark Sowul Feb 24 '11 at 14:52
@Mark Sowul thank you for put me into eric lippet. has interest ! – PRASHANT P Feb 28 '11 at 19:16
You're welcome. His blog is an invaluable resource for C# developers (and he is pretty active on Stack Overflow as well!) – Mark Sowul Feb 28 '11 at 19:18

score 1 · Answer 5 · answered Feb 23 '11 at 18:56

1

Even without fully knowing the context, I can say I would hate to be the next programmer on that code.

LINQ could probably simplify it some. If you're using ReSharper, it will suggest it for you if it can.

Beyond that, I would put some more thought into how I was getting to these nested loops in the first place and try to get rid of as many as possible. A better object model or a smart use of patterns could get you a better and more maintainable solution.

answered Feb 23 '11 at 18:56

Austin Salonen

49,173
15
109
139

present legacy codes are of poor design but refractoring too dangerous – PRASHANT P Feb 23 '11 at 20:48
@PRASHANT P: Perhaps a good task would be to make refactoring less dangerous. – Austin Salonen Feb 23 '11 at 21:13
@Austin Salonen i am not disagree,, however is not decision i am inside. much time for test is not here – PRASHANT P Feb 23 '11 at 21:23
1

@PRASHANT P: This a perfect example of a broken window. If it isn't fixed now, when will it get fixed? You stated via a misunderstanding on another answer that *your* code does not smell; by propagating this code and having your name in source control, however, *your* code does smell. – Austin Salonen Feb 23 '11 at 21:39
@Austin Salonen you are correct in ideal.. if i have termination for being outside policy of company with code of not bad smell i am not happy – PRASHANT P Feb 23 '11 at 22:27
i do not put out argumentation.. i am only say with regrets that i must have not got solution and have smell – PRASHANT P Feb 23 '11 at 22:29

score 0 · Answer 6 · answered Feb 23 '11 at 18:30

How about:

while(CONDITION)
{
  while(CONDITITION2)
  {
    if(CONDITION3)
      break CONDITION2

    if(CONDITION4)
      break CONDITION2
  }
  if(CONDITION4)
    break CONDITION
}

You could also try to work the breaking conditions into the while conditions.

Simplify nested loop logic

6 Answers6