Avoiding casting multiple times.

Question

Hello guys.
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?

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);
}

Answer 2

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

Answer 3

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);  
}  
// ...  

}

Answer 4

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.

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)
{
   ...
}

Answer 5

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.

Answer 6

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.