Tuesday, July 13, 2010

Follow up: Eliminating redundant eventhandlers using a dictionary to map controls

Yesterday I blogged on 'WebForms refactoring: Eliminating redundant eventhandlers using a dictionary to map controls'. In this post I went from bad code to better code. A smart reader gave me some good pointers on how to improve this code some more. Another iteration was necessary.

Losing the Linq

In the previous iteration I used Linq to search my dictionary.

  1:  var res = mappings.Where(map => map.Key.Equals((CheckBox)sender)).First();

This is overhead, because the dictionary implements an indexer.

   1:  TextBox[] textBoxes = mappings[checkBox];

Eliminating unnecessary casts

When I got the right dictionary entry I casted the value of the entry to an array of textboxes.

   1:  TextBox[] textBoxes = (TextBox[])res.Value;

Casting the value is unnecessary because I'm using a generic dictionary!

   1:  TextBox[] textBoxes = mappings[checkBox];

Saving a few CPU cycles

I only need to do something with my textboxes when my checkbox is not checked. Evaluating the Checked property of the sender before doing something prevents executing useless code.

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

An extra pair of eyes

Thanks for pointing out the things that were still wrong. These improvements have taken this code snippet to another level again. This also proves how having an extra pair of eyes can improve the quality of your code significantly!

I'm pretty sure there is still room for some more optimizations?

4 comments:

  1. There's more redundant stuff.

    First: there's no need to have the textBoxes variable. Just do foreach (var textBox in mappings[checkBox]) {}.

    Second: you can skip the new TextBox[] { ... } on the array construction and just use new [] { ... }.

    Also: the else is not necessary. Right now, there's no real benefit in switching the if statement. Since you're return-ing in the then block, you can put the contents of the else block on the root level of the method.

    Finally: move the mappings declaration to a Load event. There's no need to declare it everytime.

    ReplyDelete
  2. Really loved these refactoring....keep on posting more .. I'm bookmarking the article.

    ReplyDelete
  3. @Anonymous

    First: This doesn't really bother me. I like to think it improves readability a bit.
    Second: I agree.
    Also: Absolutely.
    Finally: I guess this is overhead. When we move it to the load event it gets declared everytime. Rememember this is a WebForms page.

    @Suresh

    Thanks!

    ReplyDelete
  4. Very interesting and inspires to do some refactoring in my Spaghetti-Fiestasssss
    tom

    ReplyDelete