4
\$\begingroup\$

I have a CheckChanged method that I would like to improve as it takes a lot of space in my program and really hurts readability.

The method prompts the user for input "Comments" and that is stored. However the below code is being used up to 70 times so any way to optimize would be great.

  private void checkBox45_CheckedChanged(object sender, EventArgs e)
    {
        if (checkBox45.Checked == true)
        {
            // Create an instance of the dialog
            frmInputBox input = new frmInputBox();
            // Show the dialog modally, testing the result.
            // If the user cancelled, skip past this block.
            if (input.ShowDialog() == DialogResult.OK)
            {
                // The user clicked OK or pressed Return Key
                // so display their input in this form.
                hash.Add("5", input.txtInput.Text);
                problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
                this.txtProblems2.Text = problems;
                txtProblems2.Visible = true;
            }

            // Check to see if the dialog is still hanging around
            // and, if so, get rid of it.
            if (input != null)
            {
                input.Dispose();
            }
        }
    }






  private void checkBox47_CheckedChanged(object sender, EventArgs e)
    {
        if (checkBox47.Checked == true)
        {
            // Create an instance of the dialog
            frmInputBox input = new frmInputBox();
            // Show the dialog modally, testing the result.
            // If the user cancelled, skip past this block.
            if (input.ShowDialog() == DialogResult.OK)
            {
                // The user clicked OK or pressed Return Key
                // so display their input in this form.
                hash.Add("26", input.txtInput.Text);
                problems = problems + "24. Service Access : " + input.txtInput.Text + Environment.NewLine;
                this.txtProblems5.Text = problems;
                txtProblems5.Visible = true;
            }

            // Check to see if the dialog is still hanging around
            // and, if so, get rid of it.
            if (input != null)
            {
                input.Dispose();
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ What do you mean by "used up to 70 times"? Do you mean there's a checkBox44_CheckedChanged and a checkBox46_CheckedChanged? Also, the Visual Studio tag is not relevant (as stated: "THIS TAG SHOULD ONLY BE USED FOR CODE INVOLVING THE IDE ITSELF, NOT FOR CODE SIMPLY WRITTEN WITH IT."), more relevant would be a tag that informs us of the UI (WinForms, ASP.NET,...). \$\endgroup\$ – BCdotWEB Jun 3 '15 at 9:10
  • \$\begingroup\$ Your question is not completely clear to me. What do you mean with being used up to 70 times? Is the same code being used in other events? If so, what is the difference between them? Provide more code please. \$\endgroup\$ – Abbas Jun 3 '15 at 9:10
  • \$\begingroup\$ Sorry I should have been more clear. There are lots of checkedchange events with this same code in each one, I will remove the visual studio tag \$\endgroup\$ – Lussos92 Jun 3 '15 at 9:27
  • \$\begingroup\$ Like I asked: is this code in the other events exactly the same code or are there any differences? No matter how small. \$\endgroup\$ – Abbas Jun 3 '15 at 9:33
  • \$\begingroup\$ The differences are Where the hashtable is adding (Hash.Add(") And the text at problems = problems + "5. Check Auger" changes each time, but the input is coming from the same place \$\endgroup\$ – Lussos92 Jun 3 '15 at 9:40
7
\$\begingroup\$

Before thinking I didn't read well, I didn't want to rewrite everything: the final edit is at the bottom of the answer, but please read everything to understand it all. :)


If the code inside the CheckedChanged events for all the checkboxes is exactly the same, you should extract that code and place it in a method. Then, call this method from the events:

private void ProcessInput(CheckBox cb)
{
    if (cb.Checked)
    {
        var input = new frmInputBox();

        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add("5", input.txtInput.Text);
            problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
            this.txtProblems2.Text = problems;
            txtProblems2.Visible = true;
        }

        if (input != null)
        {
            input.Dispose();
        }
    }
}

//Example usage:
private void checkBox45_CheckedChanged(object sender, EventArgs e)
{
    ProcessInput(checkBox45);
}

private void checkBox1000_CheckedChanged(object sender, EventArgs e)
{
    ProcessInput(checkBox1000);
}

Another way of achieving the same is creating one event and assign this event to the CheckedChanged event of any checkbox that requires this:

private void AnyCheckBoxChanged(object sender, EventArgs e)
{
    var cb = (CheckBox)sender;

    if (cb.Checked)
    {
        var input = new frmInputBox();

        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add("5", input.txtInput.Text);
            problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
            this.txtProblems2.Text = problems;
            txtProblems2.Visible = true;
        }

        if (input != null)
        {
            input.Dispose();
        }
    }
}

//Assigning the events:
checkBox45_CheckedChanged += AnyCheckBoxChanged;
checkBox1000_CheckedChanged += AnyCheckBoxChanged;

Other notes:

The var keyword:

From the C# Programming Guide:

The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

frmInputBox input = new frmInputBox();

would become:

var input = new frmInputBox();

Naming conventions:

Please follow the Capitalization Conventions for class names. The name of the class frmInputBox would become FormInputBox for example.

String manipulation:

This is about this line:

problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;
  1. Concatenating literal strings with variables should be done using the String.Format method:

    var result = String.Format("5. Check Auger Motor: {0}", input.txtInput.Text);        
    
  2. If this were in a loop and building large string, you'd be wasting memory. Use a StringBuilder instead (which has a method to append a string using a format, cfr. previous point):

    var builder = new StringBuilder();
    builder.AppendFormat("5. Check Auger Motor: {0}", input.txtInput.Text);
    var result = builder.ToString();
    

Disposing of the form:

Since an instance of Form is disposable, I suggest using the using statement. No need to check whether or not it can be disposed or not, at the end of the statement it's automatically disposed:

using (var input = new FormInputBox())
{            
    if (input.ShowDialog() == DialogResult.OK)
    {
        hash.Add("5", input.txtInput.Text);
        problems += String.Format("5. Check Auger Motor: {0}{1}", input.txtInput.Text, Environment.NewLine);
        txtProblems2.Text = problems;
        txtProblems2.Visible = true;
    }
}

Edit:

Since the code is not exactly the same, use the first way of solving the problem: by extracting the code into a separate method. Following code implements this:

private void ProcessInput(string hashKey, string problem, TextBox tb)
{
    using (var input = new frmInputBox())
    {
        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add(hashKey, input.txtInput.Text);
            problems += String.Format({0}: {1}{2}", problem input.txtInput.Text, Environment.NewLine);
            tb.Text = problems;
            tb.Visible = true;
        }
    }
}

//Calling of the method:
private void checkBox45_CheckedChanged(object sender, EventArgs e)
{
    if(checkBox45.Checked)
        ProcessInput("5", "5. Check Auger Motor", txtProblems2);
}

private void checkBox47_CheckedChanged(object sender, EventArgs e)
{
    if(checkBox47.Checked)
        ProcessInput("26", "24. Service Access", txtProblems5);
}

Edit2:

I re-read your updated code and noticed you set the text of a different textbox too, this is now implemented in my code example above.

\$\endgroup\$
3
\$\begingroup\$

Well, Abbas' thorough answer leaves me little to mention, but I would propose one possible improvement.

Rather than hard coding the strings in each of the CheckedChanged methods, I would create a relationship between the Checkbox objects and the strings themselves.

Declare a class to hold the lookup values:

public class ProblemData
{
    public TextBox ProblemTextBox { get; set; }
    public string HashKey { get; set; }
    public string ProblemText { get; set; }
}

Declare a Dictionary at the form level:

Dictionary<CheckBox, ProblemData> ProblemLookup = new Dictionary<CheckBox, ProblemData>();

Then, in the Form's initialize method, build the dictionary:

ProblemLookup.Add(checkbox45, new ProblemData { ProblemTextBox = txtProblems2, HashKey = "5", ProblemText = "5. Check Auger Motor" });
ProblemLookup.Add(checkbox47, new ProblemData { ProblemTextBox = txtProblems5, HashKey = "26", ProblemText = "24. Service Access" });
// ... Add one item to the dictionary for each checkbox

Then, your ProcessInput method would change to:

private void ProcessInput(TextBox problemTextBox string hashkey, string problem)
{
    using (var input = new frmInputBox())
    {
        if (input.ShowDialog() == DialogResult.OK)
        {
            hash.Add(hashKey, input.txtInput.Text);
            problems += String.Format("{0}: {1}{2}", problem, input.txtInput.Text, Environment.NewLine);
            problemTextBox.Text = problems;
            problemTextBox.Visible = true;
        }
    }
}

And you would only have a single CheckedChanged method, which all checkboxes would use as their event; like so:

private void checkBox_CheckedChanged(object sender, EventArgs e)
{
    var checkBox = sender as object;

    if(checkBox != null && checkBox.Checked && ProblemLookup.ContainsKey(checkBox))
    {
        var lookupData = ProblemLookup[checkBox];

        ProcessInput(lookupData.ProblemTextBox, lookupData.HashKey, lookupData.ProblemText);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If you look closely at the differences in the events, you'll see that the hash.Add(), the problems += ... and setting of txtProblemsX.Text are different. Your approach is nice for setting the value of problems, it only doesn't provide a solution for the other two differences. \$\endgroup\$ – Abbas Jun 3 '15 at 12:15
  • \$\begingroup\$ @Abbas, You are correct, I just ignored it.. Ha. I'll revise. \$\endgroup\$ – Wyatt Earp Jun 3 '15 at 12:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.