Messages with status codes

Question

I need to refactor following class:

public class Message
{
 public Guid ID { get; set; }
 public string MessageIn { get; set; }
 public string MessageOut { get; set; }
 public int StatusCode { get; set; } //EDIT could be changed during message lifecycle
 public bool IsDeletable
 {
 get
 {
 switch (this.StatusCode)
 {
 case 12:
 case 13:
 case 22:
 case 120:
 return true;
 default:
 return false;
 }
 }
 }
 public bool IsEditable
 {
 get
 {
 switch (this.StatusCode)
 {
 case 12:
 case 13:
 case 22:
 case 120:
 return true;
 default:
 return false;
 }
 }
 }
 public string Message
 {
 get
 {
 switch (this.StatusCode)
 {
 case 11:
 case 110:
 return this.MessageIn;
 case 12:
 case 13:
 case 22:
 case 120:
 return this.MessageOut;
 default:
 return string.Empty;
 }
 }
 }
}

1. I would like to remove the business rules IsDeletable and IsEditable
2. I would like to remove these switch statements at the same time

I am not sure if it's worth knowing that I am mapping entity to database table through Entity Framework.

One more problem that I have is that fields MessageIn and MessageOut are dependent on StatusCode. One of them are always populated. I could create new property but still the switch case is there:

public string Message
{
 get
 {
 switch (this.StatusCode)
 {
 case 10:
 case 12:
 case 13:
 case : 
 return this.MessageIn;
 default:
 return this.MessageOut;
 }
 }
 set { // switch again}
}

• radarbob: You said "you need to...", or is it just esthetics? If StatusCode is integral to a Message and edit/delete is directly related to StatusCode then it looks reasonable as is.
• Brian: public bool IsDeletable {get{return IsEditable;}} ? Currently, the two switches are identical.
• Dzenan: @Brian Yes indeed, today. Tomorrow maybe not.

Answer 1

I would like to remove these switch statments in the same time

[Flags]
public enum StatusCode{
 codeX = 1,
 codeY = 2,
 codeZ = 4,
 codeA = 8,
 editable = codeX | codeZ,
 deleteable = codeY | codeZ
}

Key points

• Use the Flags attribute
• enum values are powers of 2 - NOT multiples of 2
• bit-wise AND, OR provides the magic!

Edit

addressing the comments:

public static class StatusCodes {
 private Dictionary<StatusCode, int> values;
 private Dictionary<int,StatusCode> keys;
 static StatusCodes() {
 values = new Dictionary<StatusCode, string> {
 {StatusCode.A, 10},
 {StatusCode.B, 20},
 // and so on
 }
 keys = new Dictionary<in, StatusCode> {
 {10, StatusCode.A},
 {20, StatusCode.B},
 }
 }
 public static int GetValue(StatusCode theStatusCodeKey) {} // don't forget error trapping!
 public static StatusCode GetKey(int theIntValue) {} // ditto
 public static bool Editable(StatusCode thisCode) {
 return (StatusCode.editable & thisCode) == thisCode;
 }
 public static bool Editable(int thisValue) {
 return Editable( GetKey(thisValue));
 }
}

• The definition of the codes is all in one place - that's SOLID (D = DRY, don't repeat yourself)
• The definitions are in its own class - that's SOLID (S = single responsibility)
• editable and deleteable can be removed from Message - that's very SOLID (S = single responsibility)
• We know what all those integers mean - thats.. well, just good programming.
• The StatusCodes are available anywhere and everywhere in the application - That's SOLID, an attribute of being DRY.
• I'm not sure what "status code values might not be fixed" in the comments means. Surely the set of status codes is finite.

Edit

• Define "editability" in StatusCodes
• Use above to express Message is editable
• Address issue of exposing StatusCode.editable "as if it were a valid code"
• Adhere to Single Responsibility
• Adhere to DRY principle

...

public static class StatusCodes
{
 private static Dictionary<StatusCode, int> values;
 private static Dictionary<int,StatusCode> keys;
 static StatusCodes() {
 values = new Dictionary<StatusCode, int> {
 {StatusCode.A, 10},
 {StatusCode.B, 20},
 {StatusCode.C, 30},
 {StatusCode.D, 40}
 // and so on
 };
 keys = new Dictionary<int, StatusCode> {
 {10, StatusCode.A},
 {20, StatusCode.B},
 {30, StatusCode.C},
 {40, StatusCode.D}
 };
 }
 [Flags]
 enum Fungability
 {
 Editable = StatusCode.A | StatusCode.B,
 Deleteable = StatusCode.B | StatusCode.D
 }
 public static int GetValue( StatusCode theStatusCodeKey ) {
 int retVal;
 values.TryGetValue( theStatusCodeKey, out retVal );
 return retVal;
 } // don't forget error trapping!
 public static StatusCode GetKey( int theIntValue ) {
 StatusCode retVal;
 keys.TryGetValue( theIntValue, out retVal );
 return retVal;
 } // ditto
 public static bool Editable( StatusCode thisCode )
 {
 return ( (StatusCode)Fungability.Editable & thisCode ) == thisCode;
 }
 public static bool Editable( int thisValue )
 {
 return Editable( GetKey( thisValue ) );
 }
}
public class Message
{
 public StatusCode myStatus;
 public Message( int statusCode = 20 ) { myStatus = StatusCodes.GetKey(statusCode); }
 public Message( StatusCode statusCode = StatusCode.A ) { myStatus = statusCode; }
 public bool Editable
 {
 get { return StatusCodes.Editable( myStatus ); }
 }
 public bool Deleteable
 {
 get { return StatusCodes.Deleteable( myStatus ); }
 }
}

Take Away

• Structure data in an OO way
• Expose the data adhering to the Single Responsibility principle
• You get DRY as a side effect
• Structure yields simplicity, coherence, clarity. Editable is implemented with only one line of code!
• Message.Editable went from originally "calculating" if the status code was editable, to simply asking the StatusCode "are you editable?"

• Bobson: This only works if the status code values aren't fixed. They might be set to correspond to external values.
• MattDavey: It's a good idea but it does look like the values correspond to an external specification. They are are too arbitrary otherwise.
• radarbob: well, these codes should be defined somewhere, somehow. Use Dictionarys to map them to/from the StatusCode enumeration. The case statements, along with the redundancy and maintenance, are gone; and as a bonus we know what those integers mean!
• nmclean: This makes editable look like a valid status code. I mean, you could write message.myStatus = StatusCode.editable, which would assign multiple codes, when only one code is actually needed for it to be considered "editable". How about a separate enum like enum StatusCodeGroup { editable = StatusCode.codeX | StatusCode.codeZ ... }? The check would then be StatusCodeGroup.editable.HasFlag((StatusCodeGroup)myStatus).
• icktoofay: Having both keys and values defined like that doesn't look very DRY to me. Granted, they are in the same place, but it's still very possible to update one and forget to update the other. I'd add a little code to programmatically reverse the mapping so there really is one place where you can change the codes.

Answer 2

If status code of a message does not change during the lifecycle of Message instance, you probably can do a Replace Conditional with Polymorphism refactoring. Thus you will get an hierarchy of classes that exposes common fields. The consumers of Message class will be working with a base class, unaware of exact implementation. Exact message classes could be created via Factory Method in a base class. This approach lets you give your message classes meaningful names which would be appreciated by the future developers as they will not have to guess what that mysterious 112 status code means. Please see example below:

public abstract class MessageBase
{
 public Guid ID { get; set; }
 public string MessageIn { get; set; }
 public string MessageOut { get; set; }
 public int StatusCode { get; set; }
 public static MessageBase CreateMessage(int statusCode)
 {
 switch (statusCode)
 {
 case 12:
 case 13:
 case 22:
 case 120:
 return new MessageA();
 default:
 return new MessageB(); 
 }
 }
 public virtual bool IsDeletable
 {
 get { return false; }
 }
 public virtual bool IsEditable
 {
 get { return false; }
 }
 public virtual string Message
 {
 get
 {
 return string.Empty;
 }
 }
}
public class MessageA : MessageBase
{
 public override bool IsDeletable
 {
 get { return true; }
 }
 public override bool IsEditable
 {
 get { return true; }
 }
 public override string Message
 {
 get { return MessageOut; }
 }
}
public class MessageB : MessageBase
{
 public override bool IsDeletable
 {
 get { return true; }
 }
 public override string Message
 {
 get { return MessageIn; }
 }
}

• Dzenan: In this case status code could change during the lifecycle of Message instance.
• Andrei Zubov: In that case you may encapsulate message retrieving and deletable/editable logic in StatusCode class just like Robert suggested. In any case you definitely should have a class that represents various types of Statuses. Avoid using numbers and magic constants. It's usually hard to comprehend and maintain.

Answer 3

To adhere to the Open/Closed Principle for this class, you can either:

1. Change StatusCode to a class with bool IsEditable, bool IsDeletable, and bool?UsesOutMessage gettable only properties, generating the instances via a factory class, OR
2. Pass Func<bool, StatusCode> IsEditable, Func<bool, StatusCode> IsDeletable, and Func<bool?, StatusCode> UsesOutMessage functions into StatusCode's constructor.

I don't see any other SOLID principles being broken in your original code, but it can be hard to tell out of context.

EDIT

If you went with option 1, StatusCode would be a class:

class StatusCode
{
 private bool isDeletable;
 private bool isEditable;
 private bool? usesOutMessage;
 public StatusCode(bool isDeletable, bool isEditable, bool? usesOutMessage)
 {
 IsDeletable = isDeletable;
 IsEditable = isEditable;
 UsesOutMessage = usesOutMessage;
 }
 public bool IsDeletable
 {
 get { return isDeletable; }
 set { isDeletable = value; }
 }
 public bool IsEditable
 {
 get { return isEditable; }
 set { isEditable = value; }
 }
 public bool? UsesOutMessage
 {
 get { return usesOutMessage; }
 set { usesOutMessage = value; }
 }
}

Your code above would become:

public class Message
{
 public Guid ID { get; set; }
 public string MessageIn { get; set; }
 public string MessageOut { get; set; }
 public StatusCode StatusCode { get; set; } //EDIT could be changed during message lifecycle
 public bool IsDeletable
 {
 get
 {
 return this.StatusCode.IsDeletable;
 }
 }
 public bool IsEditable
 {
 get
 {
 return this.StatusCode.IsEditable;
 }
 }
 public string Message
 {
 get
 {
 if (this.StatusCode.UsesOutMessage.HasValue)
 {
 if (this.StatusCode.UsesOutMessage.Value)
 {
 return this.MessageOut;
 }
 else
 {
 return this.MessageIn;
 }
 }
 else
 {
 return string.Empty;
 }
 }
 }
}

The factory class would still need to do something like:

StatusCode redAlert = new StatusCode(false, true, null);
StatusCode amberAlert = new StatusCode(false, false, true);
StatusCode chartreuseAlert = new StatusCode(true, true, false);

It looks like this is just moving the logic out of MessageBase into a factory, but this adheres to the Open/Closed principle for MessageBase. MessageBase and its unit test now no longer need to be modified when new status codes are added, removed, or modified. It just works now matter how the status codes change.

This approach also keeps the logic in one place: the factory. Everything that needs to be known about status codes can be found in the factory rather than spread across multiple classes which use status codes.

• Dzenan: Could you provide code sample illustrating solution?
• Robert Gowland: @Dzenan, sure thing. See the edit.

Answer 4

When ever you encounter multiple conditions that you see growing, that should be a clear indication that a strategy pattern could help. You start by doing the following.

Create a simple enum for your status codes.

public enum MessageStatusCodes{
 case12= 12,
 case13= 13,
 case22 = 22,
 case120 = 120
}

Create your interface that will instruct your concrete strategy classes how to handle your messages

public interface IMessageStrategy{
void Process(Message);
}

Here your Message class will become your context object.

public class Message
{
public Guid ID { get; private set; }
public string MessageIn { get; private set; }
public string MessageOut { get; private set; }
public MessageStatusCodes StatusCode { get; set; } //EDIT could be changed during message lifecycle
public Message(Guid id, string messageIn, string messageOut, MessageStatusCodes statusCode){
ID=id;
MessageIn=messageIn;
MessageOut=messageOut;
StatusCode = statusCode;
LoadStrategies();
}
private static Dictionary<MessageStatusCodes, IMessageStrategy> messageStrategies;
private void LoadStrategies()
{
 messageStrategies = new Dictionary<MessageStatusCodes,IMessageStrategy>();
 messageStrategies.Add(MessageStatusCodes.case12, new DeleteMessageStrategy() );
}
 public void Process()
{ 
 return messageStrategies[StatusCode].Process(this);
}
}

Then you start by creating your concrete classes that implement the different actions or strategies that you want to perform for each condition. For example, the Delete message action could be a strategy.

public class DeleteMessageStrategy : IMessageStrategy {
public void Process(Message){
//do something based on delete status code
}
}

Then you call your Message like so

Message message = new Message(....);
message.Process();

And then your code will process the message with respect to the status code passed in.

When you're ready to expand, you implement new strategies and add those new strategies to your Message class so its aware of them.

You can really expand on this template I have shown here. Take a look at my post, I did something very similar and had questions and came to codereview for answers

My Code Review Question