C# enums and booleans - looking for a more elegant way

Question

I have an class which has a enum property and a boolean property, based on that it calls a specific method with specific parameters. I use a switch statement for the enum and an if for the boolean within each case of the switch. It is a long list and doesn't feel to me to be the most elegant solution. Anyone got a more elegant or simpler way to implement this?

            switch (ReadDecision)
            {
                case ReadDecisions.ReadNext:
                    {
                        if (UseTimeout)
                        {
                            Message = queue.Receive(Timeout);
                        }
                        else
                        {
                            Message = queue.Receive();
                        }
                        break;
                    }
                case ReadDecisions.PeekNext:
                    {
                        if (UseTimeout)
                        {
                            Message = queue.Peek(Timeout);
                        }
                        else
                        {
                            Message = queue.Peek();
                        }
                        break;
                    }
                case ReadDecisions.ReadMessageId:
                    {
                        if (UseTimeout)
                        {
                            Message = queue.ReceiveById(Id, Timeout);
                        }
                        else
                        {
                            Message = queue.ReceiveById(Id);
                        }
                        break;
                    }
                case ReadDecisions.PeekMessageId:
                    {
                        if (UseTimeout)
                        {
                            Message = queue.PeekById(Id, Timeout);
                        }
                        else
                        {
                            Message = queue.PeekById(Id);
                        }
                        break;
                    }
                case ReadDecisions.ReadCorrelationId:
                    {
                        if (UseTimeout)
                        {
                            Message = queue.ReceiveByCorrelationId(Id, Timeout);
                        }
                        else
                        {
                            Message = queue.ReceiveByCorrelationId(Id);
                        }
                        break;
                    }
                case ReadDecisions.PeekCorrelationId:
                    {
                        if (UseTimeout)
                        {
                            Message = queue.PeekByCorrelationId(Id, Timeout);
                        }
                        else
                        {
                            Message = queue.PeekByCorrelationId(Id);
                        }
                        break;
                    }
                default:
                    {
                        throw new Exception("Unknown ReadDecisions provided");
                    }
            }

Answer 1

Replace the enum with classes:
http://www.refactoring.com/catalog/replaceTypeCodeWithClass.html

You might also want to take a look at the Strategy pattern:
http://www.dofactory.com/Patterns/PatternStrategy.aspx

Answer 2

Depending on what queue is, could you change the signature of Peek() and Peek(bool) to Peek(bool?)? (The rest of the methods should follow as well.)

That way, instead of:

if (UseTimeout)
{
    Message = queue.Peek(Timeout);
}
else
{
    Message = queue.Peek();
}

you could have:

Message = queue.Peek(UseTimeout ? Timeout : null);

The other idea is you could push the decision structure to the queue class:

if(UseTimeout)
    Message = queue.PerformAction(ReadDecision, Timeout)
else
    Message = queue.PerformAction(ReadDecision)

Answer 3

The pattern I'm seeing here is a lot of method pairs named foo(ID) and foo(ID, timeout). In your situation I would:

• Create a third kind of method foo(ID, timeout, bool useTimeout), by merging the bodies of foo(ID) and foo(ID, timeout).
• Change foo(ID) to call foo(ID, null, false) and foo(ID, timeout) to call foo(ID, timeout, true)
• Change your switch statement to always call foo(ID, Timeout, UseTimeout).
• Repeat the above for each applicable pair of methods.

By doing this you've moved the flag that controls the usage of timeouts closer to where it actually needs to be used.

Answer 4

What you've got looks fine to me. It's understandable to read, I'd feel confident maintaining it. There's no need to go fancy-shmancy on it.

For example, using a fancy pattern as suggested by somebody else about using classes instead of your enum would probably result in a new class for each case statement, and they'd probably go into a file each, and you'd need another file for the base interface... and this would be a heck of a lot of stuff. I don't know how anybody could justify this heck of a lot of stuff as a simpler way to express what you've already done quite a good job of.

Answer 5

An often used convention is that a timeout of zero means no timeout. Maybe you could drop the UseTimeout (property?) and use the value zero instead. That'd eliminate some stuff.

Answer 6

I would use a Dictionary containing the enum as a Key and the Actions you do as a Value:

Dictionary<ReadDecisions,YourAction> decisions = new
           Dictionary<ReadDecisions,YourAction>();

decisions[ReadDecisions.ReadNext] = queue.Receive;
decisions[ReadDecisions.PeekNext] = queue.PeekById;
// you understand the main idea
//....

then replace your switch with:

if(UseTimeout)
   decisions[ReadDecision](id, Timeout);
else
   decisions[ReadDecision](id, 0);  //or another value 
                                    //telling you there is no timeout .
                                    // it just have to be the same signature

the only problem is that you have to have all the methods with the same signature.

Answer 7

Could you use a hashtable of delegates where the key would be the combination of ReadDecision and UseTimeout?
I have a feeling 'm asking for a downvote here...

Answer 8

Why not simplify the parameters passed

int? optionalTimeout = UseTimeout ? Timeout : null;

switch (ReadDecision) {
    case ReadDecisions.ReadNext:
        Message = queue.Receive( optionalTimeout ); break;

    case ReadDecisions.PeekNext:
        Message = queue.Peek( optionalTimeout ); break;

    // and so on ...

Then in your queue method handle a null value passed for the Timeout as "don't timeout"

Answer 9

I like Scott's suggestion to have a zero timeout mean no timeout. That would result in the following code, which is much more readable, but doesn't involve creating any extra classes. OTOH, if you've got this same switch statement anywhere else in your code, I'd go for the enum->classes refactoring.

switch (ReadDecision)
 {
  case ReadDecisions.ReadNext:
   {
    Message = queue.Receive(Timeout);
    break;
   }
  case ReadDecisions.PeekNext:
   {
    Message = queue.Peek(Timeout);
    break;
   }
  case ReadDecisions.ReadMessageId:
   {
    Message = queue.ReceiveById(Id, Timeout);
    break;
   }
  case ReadDecisions.PeekMessageId:
   {
    Message = queue.PeekById(Id, Timeout);
    break;
   }
  case ReadDecisions.ReadCorrelationId:
   {
    Message = queue.ReceiveByCorrelationId(Id, Timeout);
    break;
   }
  case ReadDecisions.PeekCorrelationId:
   {
    Message = queue.PeekByCorrelationId(Id, Timeout);
    break;
   }
  default:
   {
    throw new Exception("Unknown ReadDecisions provided");
   }
  }

Answer 10

I can suggest the following :

1] The 'pair' methods can be grouped together

Each of the individual should be able to handle NULL value. So you will have the following two groups:

Queue.Peek(TimeOut)
Queue.Receive(TimeOut)

Queue.ReceiveById(Id, TimeOut)
Queue.PeekById(Id, TimeOut)
...

2] Declare two delegates for the method groups

delegate Message MethodType1(Timeout)
delegate Message MethodType2(Id, TimeOut)

3] Have a GetDelegate() method that would return the appropriate method to execute

object GetDelegate(ReadDecisions param)
{
 switch(param)
 {

   case ReadNext:
             MethodType1 receiveDlgt = new MethodType1(queue.Receive);
   case PeekMessageId:
             MethodType2 peekDlgt = new MethodType2(queue.Peek);
   ...
 }
}

4] Make the appropriate call depending on delegate type

InvokingMethod()
{
  object methodToExecute = GetDelegate(ReadDecision)
  if (methodToExecute.GetType() == typeof(MethodType1)) 
    {
    methodToExecute(TimeOut)
    }
    else
    {
    methodToExecute(Id, TimeOut)
    }

}

Hope this helps. Some parts may need refining as per good programming practices.

Answer 11

You probably don't want to read the q like you are doing. Maybe refactor the thing to use a callback ? MessageQueue queue = new MessageQueue(queuePath); // event handler queue.ReceiveCompleted += QueueMessageReceived; // name of method that gets called // start "listening" for messages queue.BeginReceive();