Refactoring a series of if-return statements.

Question

In my model, I have things called Reports that have known workflow associated with them.

I've got a requirement to output at which state a report currently is.

public abstract partial class Report{
   public virtual string StateStatus(){
     if (Check.IsSuccessful) return "Approved";
     if (Check.AreClarificationsSent) return "Clarifications";
     if (Check.IsInProgress) return "Check in progress";
     if (IsTemplateSent) return "Sent";
     if (IsReceived) return "Received";
     return "Not received";
   }
  ...
}

I dislike this method. It won't break, but leaves some nausea in my head, just doesn't feel right.

Got any recommendations for improvement?

Answer 1

It seems that you are wanting your Check enum to be able to provide a little more information for you without the messiness of the if-then clutter. What about combining the status strings with the enumerator? Something like:

public class CheckStatus{
    public static readonly CheckStatus 
      IsSuccessful = new CheckStatus { Value = "Approved" },
      AreClarificationsSent = new CheckStatus { Value = "Clarifications" },
      IsInProgress = new CheckStatus { Value = "Check in progress" },
      IsTemplateSent = new CheckStatus { Value = "Sent" },
      IsReceived = new CheckStatus { Value = "Received" },
      NotReceived  = new CheckStatus { Value = "Not Received" };

    private CheckStatus() { }
    public string Value{ get; private set; }
}

Very easy to maintain and puts the minor implementation details behind the purpose of the Check enumerator. Provides much the same benefit of an enumeration. I would have liked to make this a struct, but I would not (without going into IL) have a parameterless private ctor.

Answer 2

I've used StringEnum as defined here several times and it works quite nicely. It works much like IAbstract's solution, but without needing to implement a separate class.

Then, wherever you need to reference the StringValue you can just use

enum Check 
{
  [StringValue("Check in progress")] InProgress,
  [StringValue("Not yet received")] NotReceived  
}

Check c = Check.InProgress;
string text = StringEnum.GetStringValue(c);

To avoid expansive rewrites you may want to use this approach.

Answer 3

A state machine. This tidies up all of those messy methods into a single property which stores the current state of your object. The downside to this is you'll need to tidy up your other objects, too...

public partial class Report {

    public enum State {
        NotReceived = 0,
        Received = 1,
        TemplateSent = 2,
        InProgress = 3,
        ClarificationSent = 4,
        Successful = 5
    }

    public State Status { get; protected set; }
}

Answer 4

Add check state enum and create Check.State property.

Same for the report.

Then your method will look like (pseudo):

{
    var ret = report.State.ToString();
    if (Check.State != CheckState.Unprocessed)
    {
        ret = Check.State.ToString();
    }

    return ret;
}

or (ugly):

return (Check.State == CheckState.Unprocessed) ? State.ToString() : Check.State.ToString();