2
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Are those calculated properties you are checking? \$\endgroup\$ – ChaosPandion Aug 4 '11 at 13:53
  • \$\begingroup\$ @ChaosPandion For calculating something, I'm always using methods. Properties are static, with already known values. \$\endgroup\$ – Arnis Lapsa Aug 4 '11 at 14:39
  • \$\begingroup\$ can it so happen, that all Check.xxx properties are true? If not is there a logic, which implements radio-button-like functionality - i.e. changing one of these changes the rest? \$\endgroup\$ – Sunny Aug 4 '11 at 16:23
  • \$\begingroup\$ @Sunny No, Check.IsSuccessful and Check.IsInProgress are mutually exclusive. However - Check.IsInProgress and Check.AreClarificationsSent can be true simultaneously. StateStatus() method should give priority to "Clarifications". What exactly that changes, what are You thinking? \$\endgroup\$ – Arnis Lapsa Aug 4 '11 at 20:57
  • \$\begingroup\$ I was thinking for a Status property, which changes whenever some of these change - i.e. move the proper logic where it belongs. It could be not good fit for you. But check my answer for another possible solution. \$\endgroup\$ – Sunny Aug 5 '11 at 13:07
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This ain't bad stuff. Using class enums myself. But that is still enum which couples all these vaguely related processes together too tightly. Also - Check is object for itself - it is responsible for checking [un]success of report. IsTemplateSent, IsReceived flags belong to report not its check (report template is generated, then - received filled out and that triggers report check to be available). It makes no sense for Check to be responsible for state of Report. \$\endgroup\$ – Arnis Lapsa Aug 4 '11 at 20:52
  • \$\begingroup\$ @Arnis L. - Now what if we renamed it CheckStatus and attached the instance to the Check object? I would also rename the instance property to Value. \$\endgroup\$ – ChaosPandion Aug 4 '11 at 20:58
  • \$\begingroup\$ @ChaosPandion IsTemplateSent belongs to report not check. It is not status of check. \$\endgroup\$ – Arnis Lapsa Aug 5 '11 at 7:59
  • \$\begingroup\$ @Arnis: Both, IsTemplateSent and IsReceived looked like bool members of Report. Remove those from CheckStatus if you like. In your original sample, the problem is that the status returned by IsTemplateSent will overwrite any return value from CheckStatus. The resulting values "Received" and "Not Received" do make sense as return status values - obviously, you can make whatever changes you like. \$\endgroup\$ – IAbstract Aug 5 '11 at 12:11
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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; }
}
\$\endgroup\$
  • \$\begingroup\$ Have thought about this. #1 InProgress means that report check is in progress and not report itself. I don't want to pull out knowledge about check outside of it. #2 this increases coupling. I really don't want to think about sending report template and processing clarifications simultaneously that this enum would force me to. So I kind a dislike this approach equally much. \$\endgroup\$ – Arnis Lapsa Aug 4 '11 at 15:07
1
\$\begingroup\$

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();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.