Refactoring an enable/disable button toggle function.

Question

I have the following simple function:

    private void EnableDisable941ScheduleBButton()
 {
  if (this._uosDepositorFrequency.Value != null)
   this._btnScheduleB.Enabled = ((int)this._uosDepositorFrequency.Value == 0);
 }

It's a member of a Winform class which I'm trying to split into a passive view and presenter. It's obvious there is business logic tangled together with UI wiring. I'm just not sure of the best way to separate them.

To give a little context, the function is called from three locations in the form. _uosDepositorFrequency is a radiobutton group with only two buttons.

Any ideas?

Update:

Ok, maybe it isn't as obvious as I thought. The business rule states that if an employer makes semiweekly deposits (_uosDepositorFrequency.Value = 0) they are required to fill out a Schedule B form.

Answer 1

I don't think that is business logic. It looks like UI logic to me. I wouldn't change it.

Although if I were using wpf, I would bind the enabled state to the data.

Answer 2

Maybe I'm missing something, but I'm not sure I see the business logic there. Your're just enabling/disabling a button based on whether or not another control on the form has value. I don't see the need to refactor this method, it's all view logic to me.

Answer 3

It sounds like you're getting the UI logic and the business rule confused as one thing. The code you have is UI logic and shouldn't be refactored. The business rule you say is that if _btnScheduleB has a value of 0 then you need to have the user fill out the form. You're business logic should be to ensure that you have a filled out form when the _btnScheduleB is enabled before the user can continue.

Answer 4

Presenter:

    if(this._uosDepositorFrequency.Value > 0) //int objects cannot be null
         ViewData["ScheduleBRequired"] = true;

View:

    private     void Draw()
    {
            if ((bool)ViewData["ScheduleBRequired"]){
                    this._btnScheduleB.Enabled = true;
                    this._validatorScheduleB.Active = true; //required data should be checked clientside with js
            }
    }

If it is a businessrequirement, that this should be filled, it should be triggered by the presenter. the ui is responsible for following the decisions of the presenter. e.g. to require the ScheduleB or not....

Answer 5

An alternative (and I'm not saying it's a better one) is to have the selection changed event for the radio control update the model (an Employer entity?), which in turn fires some kind of DepositorChanged event that the UI subscribes to and enables/disables the schedule button accordingly. The Observer pattern, more or less.

That said, the above alternative generates lots more complexity and I would suggest leaving this to the UI code (add a comment about it or use an intermediate boolean variable to state the intent to the reader). Especially if this is the only place the rule is being used.

A side note regarding the code: it's a religious debate between prefixing the class fields with underscores or using this. instead. I think everybody agrees that using both is redundant...

Answer 6

As the others have pointed out this is keeping your UI consistent so really isn't business logic. You could remove the null check like this though.

this._btnScheduleB.Enabled = (int)(this._uosDepositorFrequency.Value ?? 0) == 0;

Answer 7

I wouldn't worry too much about refactoring 2 lines of code unless it REALLY didn't make sense in the current format.

Answer 8

First I'd like to thank all those who responded to my question.

I spent some time working on this and I believe I've come up with a solution.

First I exposed _uosDepositorFrequency.Value and _btnScheduleB.Enabled as public properties and update the view interface I also took a few moments to define an enum for the deposit frequencies.

    public bool EnableScheduleB
 {
  get
  {
   return _btnScheduleB.Enabled;
  }
  set
  {
   _btnScheduleB.Enabled = value;
  }
 }

 public DepositFrequency DepositorFrequency
 {
  get
  {
   return (DepositFrequency)_uosDepositorFrequency.Value;
  }
  set
  {
   _uosDepositorFrequency.Value = (int)value;
  }
 }

Then I copied the body of the original function to my presenter and modified it to use the properties I just created. The null checking in the original function turned out to be unnecessary because the _uosDepositorFrequency control is initialized elsewhere.

    public void UpdateScheduleBStatus()
 {
  ReturnView.EnableScheduleB = ReturnView.DepositorFrequency == DepositFrequency.Semiweekly;
 }

Finally the _uosDepositorFrequency_ValueChanged event handler was updated to call UpdateScheduleBStatus.

    private void _uosDepositorFrequency_ValueChanged(object sender, System.EventArgs e)
 {
  Presenter.UpdateScheduleBStatus();
 }

Comments welcome.