HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

CheckChanged event readability

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
checkchangedreadabilityevent

Problem

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.NewLin

Solution

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;


-
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);


-
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.

Code Snippets

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);
}
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;
frmInputBox input = new frmInputBox();
var input = new frmInputBox();
problems = problems + "5. Check Auger Motor: " + input.txtInput.Text + Environment.NewLine;

Context

StackExchange Code Review Q#92549, answer score: 7

Revisions (0)

No revisions yet.