patterncsharpMinor
CheckChanged event readability
Viewed 0 times
checkchangedreadabilityevent
Problem
I have a
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
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
Another way of achieving the same is creating one event and assign this event to the
Other notes:
The
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:
would become:
Naming conventions:
Please follow the Capitalization Conventions for class names. The name of the class
String manipulation:
This is about this line:
-
Concatenating literal strings with variables should be done using the
-
If this were in a loop and building large string, you'd be wasting memory. Use a
Disposing of the form:
Since an instance of
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:
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.
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.