6
\$\begingroup\$

This code writes to Excel using the COM interface. The general issue is that any exception handling has to handle the "Excel is busy" exception. This occurs if information is sent to Excel quicker than it can handle it - eg. latency when a workbook is loaded/created, or the user is playing with the scrollbars (there are good reasons for letting this happen).

This is probably the only example I know of which is simpler and cleaner in VB6 than in C#! In VB6 an ON ERROR would be used. The error handler would then create an error for most cases. But if the error code is a "busy" then it will sleep a short period of time (typically half a second) and then try again with a "RESUME". Don't get me wrong, ON ERROR is generally messier than C#'s try...catch and it is easier to produce awful code; however, this is one example where the VB6 ON ERROR works better. A long sequence of Excel calls can be trapped with one handler. The "RESUME" will then send control back to the line where the 'busy' occurred - this avoids duplicate calls or skipped calls.

The solution I have in C# is to create a while loop with a flag. The flag indicates a repeat of the loop is required due to a 'busy' return from Excel. See the code below.

Is there a simpler, more elegant way of doing this? The main problem is that this requires a method for each type of Excel call. To avoid too many duplicate Excel calls in the busy scenario, the contents of each method is atomic or close to atomic - eg. "write this formatted value"; "apply this formatting to this row". This results in lots of methods. And/or methods with lots of parameters (the example below is a short one with just one format option, but there could be more - colors, decimal points, etc).

private void WriteDoubleValue(Excel.Worksheet sh, int x, int y, double lfval, bool bBold)
{
    bool bNotSuccess = true;
    while (bNotSuccess)
    {
        try
        {
            ((Excel.Range)sh.Cells[y,x]).set_Value(Missing.Value, lfval);
            ((Excel.Range)sh.Cells[y, x]).Font.Bold = bBold;
            bNotSuccess = false;
        }

        catch (System.Runtime.InteropServices.COMException e)
        {
            if ((e.ErrorCode & 0xFFFF) == 0xC472)
            {   // Excel is busy
                Thread.Sleep(500); // Wait, and...
                bNotSuccess = true;  // ...try again
            }
            else
            {   // Re-throw!
                throw e;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ To clarify, I'm using C# 4.0, so sepp2k's relatively advanced solution is fine. I'm a new convert from 2.0, mainly for the new multi-threading capabilities. \$\endgroup\$ – winwaed Feb 3 '11 at 16:06
7
\$\begingroup\$

If I understood you correctly, you have a lot of methods which are identical to the one you've shown except for the parameters they take and the contents of the try-block. The rest is repeated code, which is bad.

To fix this I'd recommend to abstract the "repeat this action as long as Excel is busy" logic into its own method, which takes the action to be repeated as a parameter.

On a style note, I would argue against using Hungarian Notation. It's not really commonly used in .net and basically every style guide written in this century argues against it.

I'd also recommend making the bool variable positive (i.e. success instead of notSuccess). This way people don't have to perform double negation in their head when reading things like notSuccess = false (which would be changed to success = true).

With these suggestions the code could look like this:

private void TryUntilSuccess(Action action)
{
    bool success = false;
    while (!success)
    {
        try
        {
            action();
            success = true;
        }

        catch (System.Runtime.InteropServices.COMException e)
        {
            if ((e.ErrorCode & 0xFFFF) == 0xC472)
            {   // Excel is busy
                Thread.Sleep(500); // Wait, and...
                success = false;  // ...try again
            }
            else
            {   // Re-throw!
                throw e;
            }
        }
    }
}

You could then implement WriteDoubleValue and all the methods like it with a call to TryUntilSuccess like this:

TryUntilSuccess( () =>
{
    ((Excel.Range)sh.Cells[y,x]).set_Value(Missing.Value, lfval);
    ((Excel.Range)sh.Cells[y, x]).Font.Bold = bBold;
});
\$\endgroup\$
  • \$\begingroup\$ (goes off and reads about Actions) Yes that was the type of thing I was looking for - thanks! re. Hungarian Notation: Old habits die hard! :-) re. the 'sense' of the boolean flag: Again old habits - mainly from the days when it made sense to minimize operations even in a trivial case like this. I agree that my phrasing could obfuscate slightly, and the potential performance gain is insignificant. \$\endgroup\$ – winwaed Feb 3 '11 at 15:54
  • \$\begingroup\$ I likes it. Lambdas are great. \$\endgroup\$ – Michael K Feb 3 '11 at 18:19
3
\$\begingroup\$

When re-throwing an exception don't specify the exception or throw a new exception with the old one as an inner exception otherwise you replace the exception stack trace with the line of the throw in the catch block which will prevent you from seeing which of the lines in the try block caused the exception. I assume you're using this in many places? I've refactored out the error check, but sepp2k's code promotes greater reusability of the pattern as a whole. Still, here's a slightly more efficient and shorter implementation of the control loop itself in case you're more comfortable with it. A hybrid of the two is probably your best bet.

private void WriteDoubleValue(Excel.Worksheet sh, int x, int y, double lfval, bool bBold)
{
    bool retry = false;
    do
    {
        try
        {
            ((Excel.Range)sh.Cells[y,x]).set_Value(Missing.Value, lfval);
            ((Excel.Range)sh.Cells[y, x]).Font.Bold = bBold;
            retry = false;
        }
        catch (System.Runtime.InteropServices.COMException e)
        {
            if (retry = e.ShouldRetry())
            {   // Excel is busy
                Thread.Sleep(500); // Wait, and...
            }
            else throw; 
            //calling throw without a param *rethrows* 
            //which is important to preserve the stack trace
        }
    } while (retry);
}

private void ShouldRetry(this COMException e) {
    return ((e.ErrorCode & 0xFFFF) == 0xC472);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the throw tip: Looks useful! Yes I think your while loop is more logical in its layout. Again, a case of adapting habits - I used to use do..while/repeat...until loops a lot but seemed to drift away from them - probably as I used more languages. \$\endgroup\$ – winwaed Feb 3 '11 at 19:39
  • 1
    \$\begingroup\$ The above code actually has a bug: retry should be reset either just before the try, or at the beginning of the try block. Otherwise the "busy" will cause an endless loop. \$\endgroup\$ – winwaed Feb 4 '11 at 20:22
  • \$\begingroup\$ Ahh, my apologies! Thanks for pointing it out. Such are the woes of coding in a website comment field, lol. \$\endgroup\$ – TheXenocide Feb 8 '11 at 14:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.