Tripping in Holland. Honey, who shrunk Holland?

People who follow me on Twitter might have seen my tweets about going on a weekend to Holland with some friends. Next to excessive eating and drinking, we took the time to do some sightseeing in The Hague.

The sight which gave us the most interesting pictures is Madurodam. Madurodam is a miniature city featuring the most famous Dutch buildings and landmarks. Hope you enjoy these pictures!













Most of these pictures were taken by my girlfriend.

Switching with non-constant cases in C#

Last week I came across a scenario where I wanted to switch over non-constants (aka variables), but while I was compiling I got Compiler Error CS0150 (A constant value is expected). This is one of those things I always forget. You can't use variables in your case statements because the C# compiler doesn't allow you to. It's very logical though, the compiler forces you to use constants because otherwise there is no way of knowing there are equal case statements.

The scenario

Let's say I have an ASP.NET page where the user can input and submit a value. On the server-side I want to match this value with a value from a local resource file. Depending on the match I want to execute other code.

Remember I can't use a switch because the values in the local resource file are variable.

Option one: Using else-if statements

As shown in the code snippet below you can use else-if statements to search for a match.

   1:  if (input == GetLocalResourceObject("CaseOne").ToString())

   2:  {

   3:      this.ltResult.Text = "Case one matched.";

   4:  }

   5:  else if (input == GetLocalResourceObject("CaseTwo").ToString())

   6:  {

   7:      this.ltResult.Text = "Case two matched.";

   8:  }

   9:  else if (input == GetLocalResourceObject("CaseThree").ToString())

  10:  {

  11:      this.ltResult.Text = "Case three matched.";

  12:  }

  13:  else

  14:  {

  15:      this.ltResult.Text = "No matching case found.";

  16:  }

This option has at least two disadvantages:

• It allows equal conditions (cases) which might have horrible consequences.
• It's ugly.

Option two: Using a dictionary

You can also use a generic dictionary where the keyvaluepairs have a string as the key and a delegate as the value. Because the code I'm executing when a match is found is so compact I'm using the simplest delegate of them all: an Action delegate. An Action delegate takes no parameters and does not return a value.

   1:  Dictionary<string, Action> mappings = new Dictionary<string, Action>()

   2:  {

   3:      { GetLocalResourceObject("CaseOne").ToString(), () => this.ltResult.Text = "Case one matched."},

   4:      { GetLocalResourceObject("CaseTwo").ToString(), () => this.ltResult.Text = "Case two matched."},

   5:      { GetLocalResourceObject("CaseThree").ToString(), () => this.ltResult.Text = "Case three matched."}

   6:  };

   7:   

   8:  if (mappings.ContainsKey(input))

   9:      mappings[input]();

  10:  else

  11:      this.ltResult.Text = "No matching case found.";

I think this option is a lot better than the previous one:

• This code is elegant, no spaghetti here.
• And better yet, having equal conditions (cases) is impossible. You can't add duplicate keys to a dictionary. If you do, an ArgumentException gets thrown at runtime.

More options?

Do you know more or better options?

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?

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.