Avoiding maintenance problems

My number one rule for improving code that if you can't understand it immediately after a couple of weeks without looking at the code, then nobody can. However my number two rule is that if a method doesn't fit on a page/screen then it is time it was refactored. Here's a simple example

Before

private void formatToolStripMenuItem_DropDownOpening(object sender, EventArgs e)
        {
            switch (((ColumnListEditor)_list.SelectedItems[0]).Type)
            {
                case "System.DateTime":
                    yymmddDashToolStripMenuItem.Visible = false;
                    yymmddSlashToolStripMenuItem.Visible = false;
                    yyyymmddToolStripMenuItem.Visible = false;
                    ddMMMyyDashToolStripMenuItem.Visible = false;
                    ddMMMyySlashToolStripMenuItem.Visible = false;
                    mmddyyDashToolStripMenuItem.Visible = false;
                    mmddyySlashToolStripMenuItem.Visible = false;
                    wholeNumberToolStripMenuItem.Visible = true;
                    atLeastOneDPToolStripMenuItem.Visible = true;
                    threeDecimalPlacesToolStripMenuItem.Visible = true;
                    fiveDecimalPlacesToolStripMenuItem.Visible = true;
                    nineDecimalPlacesToolStripMenuItem.Visible = true;
break; case "System.Decimal": case "System.Double": case "System.Int32": yymmddDashToolStripMenuItem.Visible = false; yymmddSlashToolStripMenuItem.Visible = false; yyyymmddToolStripMenuItem.Visible = false; ddMMMyyDashToolStripMenuItem.Visible = false; ddMMMyySlashToolStripMenuItem.Visible = false; mmddyyDashToolStripMenuItem.Visible = false; mmddyySlashToolStripMenuItem.Visible = false; wholeNumberToolStripMenuItem.Visible = true; atLeastOneDPToolStripMenuItem.Visible = true; threeDecimalPlacesToolStripMenuItem.Visible = true; fiveDecimalPlacesToolStripMenuItem.Visible = true; nineDecimalPlacesToolStripMenuItem.Visible = true; break; default: yymmddDashToolStripMenuItem.Visible = false; yymmddSlashToolStripMenuItem.Visible = false; yyyymmddToolStripMenuItem.Visible = false; ddMMMyyDashToolStripMenuItem.Visible = false; ddMMMyySlashToolStripMenuItem.Visible = false; mmddyyDashToolStripMenuItem.Visible = false; mmddyySlashToolStripMenuItem.Visible = false; wholeNumberToolStripMenuItem.Visible = false; atLeastOneDPToolStripMenuItem.Visible = false; threeDecimalPlacesToolStripMenuItem.Visible = false; fiveDecimalPlacesToolStripMenuItem.Visible = false; nineDecimalPlacesToolStripMenuItem.Visible = false; break; } }

Now what this is basically doing is showing and hiding two groups of menu items depending on a state. So what if we use Refactor Method (Ctrl-R,Ctrl-M) on first the DateTime formats and then the numeric formats

RefactorDates RefactorNumbers

Now if we make these methods take a parameter

ParameteriseNumeric ParameteriseDates

Then we can reduce the other cases, so we finally end up with

 

private void formatToolStripMenuItem_DropDownOpening(object sender, EventArgs e)
        {
            switch (((ColumnListEditor)_list.SelectedItems[0]).Type)
            {
                case "System.DateTime":
                    ShowDateFormats(true);
                    ShowNumericFormats(false);
                    break;

                case "System.Decimal":
                case "System.Double":
                case "System.Int32":
                    ShowDateFormats(false);
                    ShowNumericFormats(true);
                    break;

                default:
                    ShowDateFormats(false);
                    ShowNumericFormats(false);
                    break;
            }
        }

        private void ShowNumericFormats(bool visible)
        {
            wholeNumberToolStripMenuItem.Visible = visible;
            atLeastOneDPToolStripMenuItem.Visible = visible;
            threeDecimalPlacesToolStripMenuItem.Visible = visible;
            fiveDecimalPlacesToolStripMenuItem.Visible = visible;
            nineDecimalPlacesToolStripMenuItem.Visible = visible;
        }

        private void ShowDateFormats(bool visible)
        {
            yymmddDashToolStripMenuItem.Visible = visible;
            yymmddSlashToolStripMenuItem.Visible = visible;
            yyyymmddToolStripMenuItem.Visible = visible;
            ddMMMyyDashToolStripMenuItem.Visible = visible;
            ddMMMyySlashToolStripMenuItem.Visible = visible;
            mmddyyDashToolStripMenuItem.Visible = visible;
            mmddyySlashToolStripMenuItem.Visible = visible;
        }

Hmmm. I wonder if there is a book on this...?

 

Add comment

  Country flag


  • Comment
  • Preview
Loading