How should I refactor a long chain of try-and-catch-wrapped speculative casting operations

Question

I have some C# code that walks XML schemata using the Xml.Schema classes from the .NET framework. The various simple type restrictions are abstracted in the framework as a whole bunch of classes derived from Xml.Schema.XmlSchemaFacet. Unless there is something I've missed, the only way to know which of the derived facet types a given facet is, is to speculatively cast it to one of them, catching the resultant InvalidCastOperation in case of failure. Doing this leaves me with a really ugly function like this:

private void NavigateFacet(XmlSchemaFacet facet)
{
    try
    {
        handler.Length((XmlSchemaLengthFacet)facet);
    }
    catch(InvalidCastException)
    {
        try
        {
            handler.MinLength((XmlSchemaMinLengthFacet)facet);
        }
        catch(InvalidCastException)
        {
            try
            {
                handler.MaxLength((XmlSchemaMaxLengthFacet)facet);
            }
            catch(InvalidCastException)
            {
                ...
            }
        }
    }
}

I assume there must be more elegant ways to do this; either using some property I've missed from the .NET framework, or with some clever piece of OO trickery. Can anyone enlighten me?

Answer 1

You could try using the as keyword - if the cast fails, you get a null instead of an exception.

private void NavigateFacet(XmlSchemaFacet facet)
{
    var length = facet as XmlSchemaLengthFacet;
    if (length != null)
    {
        handler.Length(length);
        return;
    }

    var minlength = facet as XmlSchemaMinLengthFacet;
    if (minlength != null)
    {
        handler.MinLength(minlength);
        return;
    }

    var maxlength = facet as XmlSchemaMaxLengthFacet;
    if (maxlength != null)
    {
        handler.MaxLength(maxlength);
        return;
    }
    ...
}

If you had control over the classes, I'd suggest using a variant of the Visitor pattern (aka Double Despatch to recover the type information more cleanly, but since you don't, this is one relatively simple approach.

Update: Using the variable to store the result of the as cast avoids the need to go through the type checking logic twice.

Update 2: When C# 4 becomes available, you'll be able to use dynamic to do the dispatching for you:

public class HandlerDemo 
{
    public void Handle(XmlSchemaLengthFacet facet) { ... }
    public void Handle(XmlSchemaMinLengthFacet facet) { ... }
    public void Handle(XmlSchemaMaxLengthFacet facet) { ... }
    ...
}

private void NavigateFacet(XmlSchemaFacet facet)
{
    dynamic handler = new HandlerDemo();
    handler.Handle(facet);
}

This will work because method dispatch on dynamic objects uses the same rules as normal method overriding, but evaluated at run-time instead of compile-time.

Under the hood, the Dynamic Language Runtime (DLR) will be doing much the same kind of trick as the code shown in this (and other answers), but with the addition of caching for performance.

Answer 2

Hi Phil. You can try using the as keyword. Some others have recommended using the is keyword instead. I found this to be a great explanation of why as is better.

Some sample code:

private void NavigateFacet(XmlSchemaFacet facet)
{
  XmlSchemaLengthFacet lengthFacet = facet as XmlSchemaLengthFacet;
  if (lengthFacet != null)
  {
    handler.Length(lengthFacet);
  }
  else
  {
    // Re-try with XmlSchemaMinLengthFacet, etc.
  }
}

Answer 3

private void NavigateFacet(XmlSchemaFacet facet)
{
    if (facet is XmlSchemaLengthFacet)
    {
        handler.Length((XmlSchemaLengthFacet)facet);
    }
    else if (facet is XmlSchemaMinLengthFacet)
    {
        handler.MinLength((XmlSchemaMinLengthFacet)facet);
    }
    else if (facet is XmlSchemaMaxLengthFacet)
    {
        handler.MinLength((XmlSchemaMaxLengthFacet)facet);
    }

    // etc.
}

Update: I decided to benchmark the different methods discussed here (is vs. as). Here is the code that I used:

object c1 = new Class1();
int trials = 10000000;
Class1 tester;
Stopwatch watch = Stopwatch.StartNew();
for (int i = 0; i < trials; i++)
{
    if (c1 is Class1)
    {
        tester = (Class1)c1;
    }
}
watch.Stop(); 
MessageBox.Show(watch.ElapsedMilliseconds.ToString()); // ~104 ms 
watch.Reset();
watch.Start();
for (int i = 0; i < trials; i++)
{
    tester = c1 as Class1;
    if (tester != null)
    {
        // 
    }
}
watch.Stop(); 
MessageBox.Show(watch.ElapsedMilliseconds.ToString()); // ~86 ms
watch.Reset();
watch.Start();
for (int i = 0; i < trials; i++)
{
    if (c1 is Class1)
    {
        // 
    }
}
watch.Stop();     
MessageBox.Show(watch.ElapsedMilliseconds.ToString()); // ~74 ms
watch.Reset();
watch.Start();
for (int i = 0; i < trials; i++)
{
    //
}
watch.Stop();     
MessageBox.Show(watch.ElapsedMilliseconds.ToString()); // ~50 ms

As expected, using the as keyword and then checking for null is faster than using the is keyword and casting (36 ms vs. 54 ms, subtracting the cost of the loop itself).

However, using the is keyword and then not casting is faster still (24ms), which means that finding the correct type with a series of is checks and then only casting once when the correct type is identified could actually be faster (depending upon the number of different type checks that have to be done in this method before the correct type is identified).

The deeper point, however, is that the number of trials in this test is 10 million, which means it really doesn't make much difference which method you use. Using is and casting takes 0.0000054 milliseconds, while using as and checking for null takes 0.0000036 milliseconds (on my ancient notebook).

Answer 4

This is a proper way to do this without any try-catches.

if (facet is XmlSchemaLengthFacet)
{
    handler.Length((XmlSchemaLengthFacet)facet); 
} 
else if (facet is XmlSchemaMinLengthFacet)
{
    handler.MinLength((XmlSchemaMinLengthFacet)facet); 
} 
else if (facet is XmlSchemaMaxLengthFacet)
{
    handler.MaxLength((XmlSchemaMaxLengthFacet)facet); 
} 
else
{
    //Handle Error
}

Answer 5

• use "is" to determine whether an object is of a given type

• use "as" for type conversion, it is faster than normal casting and won't throw exceptions, it reuturns null on error instead

You can do it like this:

private void NavigateFacet(XmlSchemaFacet facet)
{
  if (facet is XmlSchemaLengthFacet)
  {
        handler.Length(facet as XmlSchemaLengthFacet);
  }
  else if (facet is XmlSchemaMinLengthFacet)
  {
        handler.MinLength(facet as XmlSchemaMinLengthFacet);
  }
  else if (facet is XmlSchemaMaxLengthFacet)
  {
       handler.MaxLength(facet as XmlSchemaMaxLengthFacet);
  }
}

Answer 6

Because I prefer debugging data to debugging code, I'd do it like this, especially if the code had to handle all of the XmlSchemaFacet subclasses:

Dictionary<Type, Action<XmlSchemaFacet>> HandlerMap = 
   new Dictionary<Type, Action<XmlSchemaFacet>>
{
   {typeof(XmlSchemaLengthFacet), handler.Length},
   {typeof(XmlSchemaMinLengthFacet), handler.MinLength},
   {typeof(XmlSchemaMaxLengthFacet), handler.MaxLength}
};

HandlerMap[facet.GetType()](facet);

This'll throw a KeyNotFoundException if facet isn't of a known type. Note that all of the handler methods will have to cast their argument from XmlSchemaFacet, so you're probably not saving on total lines of code, but you're definitely saving on the number of paths through your code.

There also comes a point where (assuming that the map is pre-built) mapping types to methods with a dictionary will be faster than traversing a linear list of types, which is essentially what using a bunch of if blocks gets you.