Monday, July 12, 2010

WebForms refactoring: Eliminating redundant eventhandlers using a dictionary to map controls

Last week I came across a problem which seemed trivial at first sight, but turned out to be a pretty fun refactoring challenge.

The scenario

The real problem was a bit more complex, but for simplicity's sake I made this example. In this form there are three checkboxes which map with the textboxes next to them. If the checkbox gets unchecked the corresponding textboxes should be cleared.



First iteration

I warn you. This might make your eyes bleed.

   1:  protected void chkCaseOne_CheckedChanged(object sender, EventArgs e)
   2:  {
   3:      bool isChecked = (((CheckBox)sender).Checked);
   4:   
   5:      if (!isChecked)
   6:      {
   7:          this.txtCaseOneFirst.Text = string.Empty;
   8:          this.txtCaseOneSecond.Text = string.Empty;
   9:      }
  10:  }
  11:   
  12:  protected void chkCaseTwo_CheckedChanged(object sender, EventArgs e)
  13:  {
  14:      bool isChecked = (((CheckBox)sender).Checked);
  15:   
  16:      if (!isChecked)
  17:      {
  18:          this.txtCaseTwoFirst.Text = string.Empty;
  19:          this.txtCaseTwoSecond.Text = string.Empty;
  20:      }
  21:  }
  22:   
  23:  protected void chkCaseThree_CheckedChanged(object sender, EventArgs e)
  24:  {
  25:      bool isChecked = (((CheckBox)sender).Checked);
  26:   
  27:      if (!isChecked)
  28:      {
  29:          this.txtCaseThreeFirst.Text = string.Empty;
  30:          this.txtCaseThreeSecond.Text = string.Empty;
  31:      }
  32:  }


Every developer should be able to see the horror in this Copy-Paste fiesta.

Second iteration

The obvious next step is extracting the repeated logic into a method.

   1:  protected void chkCaseOne_CheckedChanged(object sender, EventArgs e)
   2:  {
   3:      this.ClearTextBoxesIfCheckBoxNotChecked(
   4:          (CheckBox)sender,
   5:          new TextBox[] { this.txtCaseOneFirst, this.txtCaseOneSecond });
   6:  }
   7:   
   8:  protected void chkCaseTwo_CheckedChanged(object sender, EventArgs e)
   9:  {
  10:      this.ClearTextBoxesIfCheckBoxNotChecked(
  11:          (CheckBox)sender,
  12:          new TextBox[] { this.txtCaseTwoFirst, this.txtCaseTwoSecond });
  13:  }
  14:   
  15:  protected void chkCaseThree_CheckedChanged(object sender, EventArgs e)
  16:  {
  17:      this.ClearTextBoxesIfCheckBoxNotChecked(
  18:          (CheckBox)sender,
  19:          new TextBox[] { this.txtCaseThreeFirst, this.txtCaseThreeSecond });
  20:  }
  21:   
  22:  private void ClearTextBoxesIfCheckBoxNotChecked(CheckBox checkBox, TextBox[] textBoxes)
  23:  {
  24:      if (!checkBox.Checked)
  25:      {
  26:          foreach (TextBox textBox in textBoxes)
  27:          {
  28:              textBox.Text = string.Empty;
  29:          }
  30:      }
  31:  }

This is better, but I'm still not satisfied. Those three eventhandlers seem to be overhead, and are just cluttering my code-behind.

Third and final iteration

In the final iteration I get rid of the three seperate eventhandlers. I use a dictionary to create a mapping between the checkboxes and their corresponding textboxes. Using the eventhandlers sender I get the matching mapping in the dictionary. Once the matching mapping is found I extract the checkbox and its corresponding textboxes from the keyvaluepair and apply the logic.

   1:  protected void chkCase_CheckedChanged(object sender, EventArgs e)
   2:  {
   3:      Dictionary<CheckBox, TextBox[]> mappings = new Dictionary<CheckBox, TextBox[]>()
   4:      {
   5:          {this.chkCaseOne, new TextBox[] {this.txtCaseOneFirst, this.txtCaseOneSecond}},
   6:          {this.chkCaseTwo, new TextBox[] {this.txtCaseTwoFirst, this.txtCaseTwoSecond}},
   7:          {this.chkCaseThree, new TextBox[] {this.txtCaseThreeFirst, this.txtCaseThreeSecond}}
   8:      };
   9:   
  10:      var res = mappings.Where(map => map.Key.Equals((CheckBox)sender)).First();
  11:   
  12:      CheckBox checkBox = (CheckBox)res.Key;
  13:      TextBox[] textBoxes = (TextBox[])res.Value;
  14:   
  15:      if (!checkBox.Checked)
  16:      {
  17:          foreach (TextBox textBox in textBoxes)
  18:          {
  19:              textBox.Text = string.Empty;
  20:          }
  21:      }           
  22:  } 

One more iteration?

I'm pretty satisfied with the last iteration. Are you?

You can find the follow-up here.

2 comments:

  1. - i would extract the checkbox to textboxes mapping into a separate method. (you'll need it elsewhere for sure)
    - i would not use Linq, mappings[checkbox] would give textboxes already.
    - the casts on line 12,13 are not neccesary
    - i would first cast the sender once to checkbox, and use that to see if it is checked as my first two lines.

    ReplyDelete
  2. Great feedback! All points make perfect sense.

    ReplyDelete