4

EDIT

I'm leaving this up here, even though it makes me look stupid, because it's a subtle bug that can bite you if you're working late at night and not paying attention. Kudos to Visual Studio for having such an intelligent parser.

Basically I missed that I have nested loops, so the continue statement is essentially worthless here because it is continuing the foreach loop, not the for loop.

ORIGINAL QUESTION

I'm running a search on a workbook, looking for the sheet that matches all the string search criteria. In the Visual Studio editor, the i++ is underlined as "unreachable code".

/// <summary>
/// Finds the first sheet that has cells that match all the criteria.
/// </summary>
/// <param name="wb"></param>
/// <param name="searches"></param>
/// <returns></returns>
public static ISheet FindSheet( this IWorkbook wb, params string[] searches )
{
    if( null == wb || null == searches )
        return null;

    for( int i = 0; i < wb.NumberOfSheets; i++ )
    {
        var sheet = wb.GetSheetAt( i );
        foreach( var search in searches )
        {
            var cell = sheet.FindCell( search );
            if( null == cell )
                continue;
        }

        return sheet;
    }

    return null;
}

I would think that the continue statement has a clear meaning here: "If any of the search criteria return a null cell, continue to the next iteration. Otherwise, just return the sheet found in this iteration."

CORRECTED CODE WITH NO CONTINUE STATEMENT

/// <summary>
/// Finds the first sheet that has cells that match all the criteria.
/// </summary>
/// <param name="wb"></param>
/// <param name="searches"></param>
/// <returns></returns>
public static ISheet FindSheet( this IWorkbook wb, params string[] searches )
{
    if( null == wb || null == searches )
        return null;

    for( int i = 0; i < wb.NumberOfSheets; i++ )
    {
        var sheet = wb.GetSheetAt( i );
        if( searches.All( s => null != sheet.FindCell( s ) ) )
            return sheet;
    }

    return null;
}
Bruce Pierson
  • 567
  • 7
  • 11
  • This is off-topic for CodeReview, but, the continue continues the `foreach` loop, not the `for` loop ... just saying, and now voting to close – rolfl Mar 22 '14 at 03:32
  • The `return sheet;` statement always prevents it reaching the end of the `for` loop. – ChrisW Mar 22 '14 at 03:34
  • 1
    Why the Yoda conditions? – Mathieu Guindon Mar 22 '14 at 03:35
  • 2
    Hey Bruce.... welcome to CodeReview. When you add a label to your loops and use it on the continue... why don't you bring the code back for review? Seems there are a few things we can suggest.... ;-) – rolfl Mar 22 '14 at 03:36
  • No, it is not... [The target of a continue statement is the end point of the embedded statement of the nearest enclosing while, do, for, or foreach statement. If a continue statement is not enclosed by a while, do, for, or foreach statement, a compile-time error occurs.](http://msdn.microsoft.com/en-us/library/aa664757%28v=vs.71%29.aspx) – rolfl Mar 22 '14 at 03:37
  • @BrucePierson Clarification: The continue stops execution and continues to the top of the containing loop. – Casey Mar 22 '14 at 03:38
  • are you using `NPOI` ? – linquize Mar 22 '14 at 04:38
  • Yes, this is a library of extension methods for NPOI, for some ETL stuff. – Bruce Pierson Mar 22 '14 at 05:07
  • @retailcoder - got the Yoda conditions reference, I finally did. Goes back to the day when you could accidentally do `x = null` (another subtle bug), it does. – Bruce Pierson Mar 22 '14 at 05:17

2 Answers2

5

Here, your for loop in completely useless. After the first iteration, it will just return no matter what. Sure your foreach loop has a continue statement, but this only applies to the foreach loop. This is why your code is unreachable.

Because of this, only the first sheet will be analysed. If this is what you want, you can simply remove the for loop and target the Worksheet at index 0, or you need to reaarange your loop.

gretro
  • 1,934
  • 15
  • 25
  • 1
    Because the nearest loop from the continue statement is the `foreach` loop. In Java, he could have named the loop and could have specified which he wanted to continue, but you can't do it in C#. – gretro Mar 22 '14 at 03:47
  • Thanks. I posted an edit while you were answering. I marked this as the answer. – Bruce Pierson Mar 22 '14 at 03:49
  • 2
    It's by making this kind of mistakes taht you learn the mechanics of a language. Don't worry, we've all been there. ^^ – gretro Mar 22 '14 at 03:49
3

Eric Lippert has an SO answer and a blog post on what you can do when you want to continue an outer loop when you're in an inner loop.

The technique he has marked as "awesome" is to get rid of all of the loops when it's possible to do so, and indeed I believe it is possible here:

public static ISheet FindSheet( this IWorkbook wb, params string[] searches )
{
    if( null == wb || null == searches ) { return null; }

    return wb.FirstOrDefault(sh => searches.All(sr => sh.FindCell(sr) != null));
}
Community
  • 1
  • 1
JLRishe
  • 99,490
  • 19
  • 131
  • 169