Avoiding casting multiple times

Question

I have a method which receives a parameter of base type and performs some pre-processing depending on actual parameter type.
Here is my code:

public void OnMessageReceived(QuickFix42.Message message)
{
    if (message is QuickFix42.ExecutionReport)
    {
        ProcessExecutionReport(message as QuickFix42.ExecutionReport);
    }
    else if (message is QuickFix42.AllocationACK)
    {
        ProcessAllocationAck(message as QuickFix42.AllocationACK);
    }
    else if (message is QuickFix42.OrderCancelReject)
    {
        ProcessOrderCancelReject(message as QuickFix42.OrderCancelReject);
    }
    // ...
}

Everything works fine, but I get the following warning from Visual Studio:

Warning 760 CA1800 : Microsoft.Performance : 'message', a parameter, is cast to type 'ExecutionReport' multiple times in method 'MessageProcessor.OnMessageReceived(Message)'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction.

What's the best way to avoid these redundant casts?

Branch

Answer 1

Don't use both is and as. You just have to use as and check if the result is null :

QuickFix42.ExecutionReport execReport = message as QuickFix42.ExecutionReport
if (execReport != null)
{
  ProcessExecutionReport(execReport);
}

Snaggy

Doing it that way may result in a lot of unnecessary casts being done as you've changed if/else-if to separate if statements. I've posted a slightly modified version below which preserves the original structure. – Colwin

Answer 2

Given the repeating structure of your code you can clean it up in this way:

public void OnMessageReceived(QuickFix42.Message message)
{
    ExecuteOnlyAs<QuickFix42.ExecutionReport>(message, ProcessExecutionReport);
    ExecuteOnlyAs<QuickFix42.AllocationACK>(message, ProcessAllocationAck);
    ExecuteOnlyAs<QuickFix42.OrderCancelReject>(message, ProcessOrderCancelReject);
}

private void ExecuteOnlyAs<T>(QuickFix42.Message message, Action<T> action)
{
    var t = message as T;
    if (t != null)
    {
        action(t);
    }
}

This does assume that the types don't inherit from each other. If they do you'll need to change ExecuteOnlyAs to return a bool indicating success and do the if statements as before.

Muscolo

Answer 3

As others have posted, a singleasfollowed by anull-test will be quite efficient. I do notice though, that this appears to be a QuickFix app. QuickFix provides a purpose-builtMessageCrackerclass, which I am sure is efficiently implemented, to 'crack' generic FIX messages into specific strongly-typed message objects, which it appears is precisely what you want to do. The benefits of doing it this way will be more than just (probably) improved performance; the code will be cleaner (less checks and casts, and the per-message handling code will be naturally moved to the appropriate handler) and more robust in the face of invalid messages.

EDIT: A look at the source of MessageCracker, as prompted by John Zwinck, puts paid to the idea that using this class will probably result in better performance.

Example: (make your class inherit fromMessageCracker)

public void OnMessageReceived(QuickFix42.Message message, SessionID sessionID)
{
  crack (message, sessionID);
}

public override void onMessage(QuickFix42.ExecutionReport message, SessionID sessionID)
{
   ...
}

public override void onMessage(QuickFix42.OrderCancelReject message, SessionID sessionID)
{
   ...
}

Organzine

You say "MessageCrackerclass, which I am sure is efficiently implemented." Have you ever looked at it? It is not efficiently implemented at all--it is a giant pile of if statements, one after the other (this has been true for years, and is true as I write this). See here: quickfix.svn.sourceforge.net/viewvc/quickfix/trunk/quickfix/src/… . It's really rather shameful. At least with a hand-crafted if/else chain you only check the cases you care about (imagine if your most common messages are at the bottom of the cracker). – Intwine

@John Zwinck: I stand corrected. You're right, it is pretty bad, isn't it? – Organzine

Answer 4

QuickFix42.ExecutionReport executionReportMessage = message as QuickFix42.ExecutionReport;
if (executionReportMessage != null) 
{ 
  ProcessExecutionReport(executionReportMessage); 
} 

Mislead

Answer 5

In my opinion I think you have something missing from your design. Typically I've seen something similar to this before, whereby this could happen:

public interface IResult
{
  // No members
}

public void DoSomething(IResult result)
{
  if (result is DoSomethingResult)
    ((DoSomethingResult)result).DoSomething();
  else if (result is DoSomethingElseResult)
    ((DoSomethingElseResult)result.DoSomethingElse();
}

Because the contract doesn't define any operations, it forces you to perform type casting to get any benefits from it. I don't think this is correct design. A contract (be it an interface or an abstract class) should define an intended operation, and it should be clear how to use it. Your example above is essentially the same issue, Message isn't defining the way in which it should be used... you should modify Message to enforce some operation:

public abstract class Message
{
  public abstract void Process();
}

public class ExecutionReportMessage : Message
{
  public override void Process()
  {
    // Do your specific work here.
  }
}

With that, you greatly simplify your implementation:

public void OnMessageReceived(QuickFix42.Message message)
{
    if (message == null)
      throw new ArgumentNullException("message");

    message.Process();
}

It's much cleaner, more testable, and no type casting.

Crew

Answer 6

Well there are lot of ways above code can be refactored because each time you would require a new message this code has to be changed there are some well known Design Pattern which can handle this requirement but for initial starting you can do soemthing like below , it is not best way but it will remove the warning though i have not tested this code but i think so.

public void OnMessageReceived(QuickFix42.Message message)  
{  
      QuickFix42.ExecutionReport ExecutionReportMessage = message as QuickFix42.ExecutionReport;
      QuickFix42.AllocationACK  AllocationACKMessage = message as QuickFix42.AllocationACK  ;
      QuickFix42.OrderCancelReject OrderCancelRejectMessage = message as QuickFix42.OrderCancelReject;

 if (ExecutionReportMessage !=null)  
{  
    ProcessExecutionReport(ExecutionReportMessage);  
}  
else if (AllocationACKMessage !=null)  
{  
    ProcessAllocationAck(AllocationACKMessage );  
}  
else if (OrderCancelRejectMessage !=null)  
{  
    ProcessOrderCancelReject(OrderCancelRejectMessage);  
}  
// ...  

}

Fire

Answer 7

public void OnMessageReceived(QuickFix42.Message message)
{
    QuickFix42.ExecutionReport executionReport;
    QuickFix42.AllocationACK allocationAck;
    QuickFix42.OrderCancelReject orderCancelReject;

    if ((executionReport = message as QuickFix42.ExecutionReport) != null)
    {
        ProcessExecutionReport(executionReport);
    }
    else if ((allocationAck = message as QuickFix42.AllocationACK) != null)
    {
        ProcessAllocationAck(allocationAck);
    }
    else if ((orderCancelReject = message as QuickFix42.OrderCancelReject) != null)
    {
        ProcessOrderCancelReject(orderCancelReject);
    }
    // ...
}

Colwin