Unreachable Code Detected - ?

[Duki]

Hey everyone,

I have a warning message in my app saying "Unreachable code detected" - Here's a snippet:

//Files are sometimes not closed quickly enough - these loop variables allow for the system to wait and try again
        const int MAX_TRIES = 3;
        int loop = 0;

...


public void CreateSheet(Laptop lptp, string file, int id)
{
    PointOfRetry:
    try
    {
        subLog.LogMessageToFile("Opening sheet in " + file + " for asset id: " + id);

        //Open specified file and set activate worksheed according to asset ID
        xlBook = xl.Workbooks.Open(file_path + file, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing);
        xlSheet = xlBook.Worksheets.get_Item(2);

        subLog.LogMessageToFile("Creating sheet in " + file + " for asset id: " + id);

        //Create a copy of sheet #2 at the end of the workbook
        int last = xlBook.Worksheets.Count;
        xlSheet.Copy(oMissing, xl.Worksheets[last]);
        xl.Worksheets[last+1].name = id.ToString();     //rename to corresponding asset ID

        xlSheet = xlBook.Worksheets.get_Item(last + 1);
        xlSheet.Cells[2, 1] = lptp.laptop_ID;
        xlSheet.Cells[2, 2] = lptp.pc_name;
        xlSheet.Cells[2, 3] = lptp.manufacturer;
        xlSheet.Cells[2, 4] = lptp.model;
        xlSheet.Cells[2, 5] = lptp.location;

        subLog.LogMessageToFile("Saving file: " + file);
        xlBook.Save();
    }
    catch (Exception ex)
    {
        for (loop = 0; loop < MAX_TRIES; loop++) ////  << loop -> "Unreachable code detected"
        {
            MessageBox.Show("File Locked - Waiting for access...");
            Thread.Sleep(3000);
            goto PointOfRetry;
        }

        subLog.LogMessageToFile("Create Error - Source: " + ex.Source);
        subLog.LogMessageToFile("Create Error Message: " + ex.Message);
        MessageBox.Show("Create Error: " + ex.Message, "Could not create sheet", MessageBoxButtons.OK, MessageBoxIcon.Error);
    }
    finally
    {
        subLog.LogMessageToFile("Closing session: " + file);

        xl.Application.Workbooks.Close();
        xl.Workbooks.Close();
        xl.Quit();
        System.Runtime.InteropServices.Marshal.ReleaseComObject(xlBook);
        System.Runtime.InteropServices.Marshal.ReleaseComObject(xlSheet);
        System.Runtime.InteropServices.Marshal.ReleaseComObject(xl);
    }
}

MAX_TRIES and loop are both private variables inside this class.

Haven't caught an exception with this code yet, so I'm not sure if it's working or not. Any thoughts?

[nick.crane]

Having this goto PointOfRetry; inside the for loop is why you have the error.
All the code beneath it (in the catch block) will never be executed.
Using goto is not very good coding practice.
I recommend you try to find another way; using a while loop perhaps

[nick.crane]

Looking at is again, it is actually the loop++ that is not executed.
Which is why the error shows at that line.

Put the try catch block inside the for (3 x retry) loop and break if OK otherwise wait and loop again.
E.G.

for (int i = 0; i < MAX_TRIES; i++)
            {
                try
                {
                    // do stuff
                    break;
                }
                catch (Exception ex)
                {
                    // report waiting
                    Thread.Sleep(3000);
                }
            }
            // do finally stuff

Edit: P.S. Exception is not only due to file error.
xlSheet = xlBook.Worksheets.get_Item(2); //<-- will fail if item 2 does not exist!

[Duki]

Thanks for the help - I'll give that a shot.

Edit: P.S. xlSheet = xlBook.Worksheets.get_Item(2); //<-- will fail if item 2 does not exist!

I know - sheet 2 is a template used for creating new sheets. It should always exist. Thanks again

[nick.crane]

Re: xl.Worksheets[last] Is Worksheets one based or zero based. If zero based then you need to sub 1.

[Duki]

I'm pretty sure it's one based - according to some posts I saw on msdn sites.

[nick.crane]

Yea, I thought it might be the case since XL scripts are in basic. You'd soon know if it was wrong anyway.

[nick.crane]

Slight mod to my original snippet.

int loop;
for (loop=0; i < MAX_TRIES; loop++)
{
    try
    {
        // do stuff
        break;
    }
    catch (Exception ex)
    {
        // report waiting
        Thread.Sleep(3000);
    }
}
if (loop == MAX_TRIES)
{
    // report error
}
// do finally stuff carefully 
// maybe need another try block
// use if(xlBook!=null) too

[Duki]

If an exception hits, my finally-stuff block won't execute if it's not part of the try-catch-finally block will it?

edit> just saw your post

[nick.crane]

The finaly block will always execute, even if you re-throw the exception.
However, if you do not re-throw, the code following the try-catch will execute.

[Geekitygeek]

for (loop=0; i < MAX_TRIES; loop++)

should be for (loop=0; loop < MAX_TRIES; loop++) right? :)

[Duki]

How about something like

int loop;
for (loop=0; loop < MAX_TRIES; loop++)
{
    try
    {
        // do stuff
    }
    catch (Exception ex)
    {
        // report waiting
        Thread.Sleep(3000);
    }
    finally
    {
        if ( loop != MAX_TRIES )
        {
            //do stuff
            break;
        }    
        else
        {
            //do stuff
        }
    }
}

See any err?

[nick.crane]

for (loop=0; i < MAX_TRIES; loop++)

should be for (loop=0; loop < MAX_TRIES; loop++) right? :)

Yeah, thank. It is only psuedo code.

Duki, you need to do the test for the error out side the loop.

int loop;
for (loop = 0; loop < MAX_TRIES; loop++)
{
    try
    {
        // do stuff
    }
    catch (Exception ex)
    {
        // report waiting
        Thread.Sleep(3000);
        continue; //<-- loop back to try again
    }
    finally
    {
        // do finally stuff carefully 
        // maybe need another try block
        // use if(xlBook!=null) too
    }
}
if (loop == MAX_TRIES)
{
    // report error
}

[Duki]

Ahh -

continue;

is something to never be forgotten, but I did :(. Thanks!

[nick.crane]

That last post does not work. The finaly block executes each re-try!
Use my other post. This works fine.

Slight mod to my original snippet.

int loop;
for (loop=0; i < MAX_TRIES; loop++)
{
    try
    {
        // do stuff
        break;
    }
    catch (Exception ex)
    {
        // report waiting
        Thread.Sleep(3000);
    }
}
if (loop == MAX_TRIES)
{
    // report error
}
// do finally stuff carefully 
// maybe need another try block
// use if(xlBook!=null) too

(or will if you correct the error Ryshad pointed out;))

[Duki]

Go it working - for completeness, here's my code as of now. If you see any errors, let me know. Thanks for the help!

public void CreateSheet(Laptop lptp, string file, int id)
{
    for (loop = 0; loop < MAX_TRIES; loop++)
    {
        try
        {
            subLog.LogMessageToFile("Opening sheet in " + file + " for asset id: " + id);

            //Open specified file and set activate worksheed according to asset ID
            xlBook = xl.Workbooks.Open(file_path + file, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing, oMissing);
            xlSheet = xlBook.Worksheets.get_Item(2);

            subLog.LogMessageToFile("Creating sheet in " + file + " for asset id: " + id);

            //Create a copy of sheet #2 at the end of the workbook
            int last = xlBook.Worksheets.Count;
            xlSheet.Copy(oMissing, xl.Worksheets[last]);
            xl.Worksheets[last + 1].name = id.ToString();     //rename to corresponding asset ID

            xlSheet = xlBook.Worksheets.get_Item(last + 1);
            xlSheet.Cells[2, 1] = lptp.laptop_ID;
            xlSheet.Cells[2, 2] = lptp.pc_name;
            xlSheet.Cells[2, 3] = lptp.manufacturer;
            xlSheet.Cells[2, 4] = lptp.model;
            xlSheet.Cells[2, 5] = lptp.location;

            subLog.LogMessageToFile("Saving file: " + file);
            xlBook.Save();

            break;
        }
        catch (Exception ex)
        {
            Thread.Sleep(3000);

            subLog.LogMessageToFile("Create Error - Source: " + ex.Source);
            subLog.LogMessageToFile("Create Error Message: " + ex.Message);                    
            continue;
        }
        finally
        {
            subLog.LogMessageToFile("Closing session: " + file);

            xl.Application.Workbooks.Close();
            xl.Workbooks.Close();
            xl.Quit();
            System.Runtime.InteropServices.Marshal.ReleaseComObject(xlBook);
            System.Runtime.InteropServices.Marshal.ReleaseComObject(xlSheet);
            System.Runtime.InteropServices.Marshal.ReleaseComObject(xl);
        }
    }

    if (loop == MAX_TRIES)
    {
        subLog.LogMessageToFile("Closing session: " + file);
        subLog.LogMessageToFile("Error Occured in session: " + file);

        MessageBox.Show("Create Error - Check log file for details", "Could not create sheet", MessageBoxButtons.OK, MessageBoxIcon.Error);

        xl.Application.Workbooks.Close();
        xl.Workbooks.Close();
        xl.Quit();
        System.Runtime.InteropServices.Marshal.ReleaseComObject(xlBook);
        System.Runtime.InteropServices.Marshal.ReleaseComObject(xlSheet);
        System.Runtime.InteropServices.Marshal.ReleaseComObject(xl);
    }

}

[Duki]

Shucks - so the continue doesn't work there?

[nick.crane]

Actually, I think it would be better to say that the finaly block is working correctly by executing no matter how you exit the try-catch block.

[Duki]

I'll mark it as solved :)
Thanks for the help!