patterncsharpMinor
Disable and enable text boxes according to the radio button click
Viewed 0 times
boxesenabletheclickdisabletextradiobuttonandaccording
Problem
Here is a GPA calculator in C#. I am feeling like the code that I write and format are quite long and would like it to be shorter. This program is using radio buttons to enable and disable the text boxes according to the number of subject chosen. Perhaps that part can be shortened.
```
public double gradePoint1, gradePoint2, gradePoint3, gradePoint4, gradePoint5, gradePoint6;
public double courseGP1, courseGP2, courseGP3, courseGP4, courseGP5, courseGP6;
int credHour1 = 0, credHour2 = 0, credHour3 = 0, credHour4 = 0, credHour5 = 0, credHour6 = 0;
public Form1()
{
InitializeComponent();
}
private void Form1_Load(object sender, EventArgs e)
{
// make the display bigger
this.Font = new Font("Arial", 15);
}
private void btnDisplay_Click(object sender, EventArgs e)
{
// to disable certain text boxes according to the number of courses taken
if (rad1.Checked)
DisplayOne();
else if (rad2.Checked)
DisplayTwo();
else if (rad3.Checked)
DisplayThree();
else if (rad4.Checked)
DisplayFour();
else if (rad5.Checked)
DisplayFive();
else
DisplaySix();
InitializeLabels();
}
private void InitializeLabels()
{
// initialize all Credit Hours labels
lblCH1.Text = "0";
lblCH2.Text = "0";
lblCH3.Text = "0";
lblCH4.Text = "0";
lblCH5.Text = "0";
lblCH6.Text = "0";
}
private void DisplayOne()
{
txtCCode1.Enabled = true;
txtCCode2.Enabled = false;
txtCCode3.Enabled = false;
txtCCode4.Enabled = false;
txtCCode5.Enabled = false;
txtCCode6.Enabled = false;
txtGrade1.Enabled = true;
txtGrade2.Enabled = false;
txtGrade3.Enabled = false;
txtGrade4.Enabled = false;
txtGrade5.Enabled = false;
txtGrade6.Enabled = false;
}
private void DisplayTwo()
{
txtCCode1.Enabled = true;
txtCCode2.Enabled = true;
txtCCode3.Enabled = false;
txtCCode4.Enabled = false;
txtCCode5.Enabled = false;
txtCCode6.Enabled = false;
```
public double gradePoint1, gradePoint2, gradePoint3, gradePoint4, gradePoint5, gradePoint6;
public double courseGP1, courseGP2, courseGP3, courseGP4, courseGP5, courseGP6;
int credHour1 = 0, credHour2 = 0, credHour3 = 0, credHour4 = 0, credHour5 = 0, credHour6 = 0;
public Form1()
{
InitializeComponent();
}
private void Form1_Load(object sender, EventArgs e)
{
// make the display bigger
this.Font = new Font("Arial", 15);
}
private void btnDisplay_Click(object sender, EventArgs e)
{
// to disable certain text boxes according to the number of courses taken
if (rad1.Checked)
DisplayOne();
else if (rad2.Checked)
DisplayTwo();
else if (rad3.Checked)
DisplayThree();
else if (rad4.Checked)
DisplayFour();
else if (rad5.Checked)
DisplayFive();
else
DisplaySix();
InitializeLabels();
}
private void InitializeLabels()
{
// initialize all Credit Hours labels
lblCH1.Text = "0";
lblCH2.Text = "0";
lblCH3.Text = "0";
lblCH4.Text = "0";
lblCH5.Text = "0";
lblCH6.Text = "0";
}
private void DisplayOne()
{
txtCCode1.Enabled = true;
txtCCode2.Enabled = false;
txtCCode3.Enabled = false;
txtCCode4.Enabled = false;
txtCCode5.Enabled = false;
txtCCode6.Enabled = false;
txtGrade1.Enabled = true;
txtGrade2.Enabled = false;
txtGrade3.Enabled = false;
txtGrade4.Enabled = false;
txtGrade5.Enabled = false;
txtGrade6.Enabled = false;
}
private void DisplayTwo()
{
txtCCode1.Enabled = true;
txtCCode2.Enabled = true;
txtCCode3.Enabled = false;
txtCCode4.Enabled = false;
txtCCode5.Enabled = false;
txtCCode6.Enabled = false;
Solution
There are quite a few issues in this code that would make it unsuitable for production/commercial use and I'll run through what we've got. Since this appears to be a Homework assignment I will only provide a small number of code samples for you, it will be up to you to implement the advice into your code.
Declaring multiple single variables is bad. There are better ways to represent your data in C#, for example look at using
The variable names are not very meaningful. When this happens it should be taken as a sign that you may have started writing code before you've fully understood the problem.
Shows us there is a mix of 'business logic' (or the rules of what need to be processed and how) and 'user interface' code. While it was once common practice in winforms code to do this, it's a bad habit that you shouldn't get into. Any code related to the processing of data should be moved out of this class into a separate file and class.
Moving the business logic to another class also means that you can test your code more easily and can write automated tests that ensure any changes later don't break something that previously worked.
For example you can create a console app which feeds the class known good (and bad) data to make sure that you get the results you expect.
Is in the wrong place as you can set the default font & size for a form via the forms designer.
The naming of this code makes me think that after selecting a radio button that users need to click a button to change the interface to allow them to enter the correct data. Check the radio button events and you'll find that they too have a 'click' event which can enable changes to the UI as you select an option.
Radio buttons in general are not especially useful in this context since users can just leave rows blank if they are not needed.
Is a situation where you could look at using something called a UserControl and a Control Array; these would allow you to stop repeating large chunks of code with minor changes as you are now.
In fact, comparing
Using code like this we can get rid of all the
Could be shortened extensively using either a
There is quite a bit more that you could do, but what's here will be plenty for now...
public double gradePoint1, gradePoint2, gradePoint3, gradePoint4, gradePoint5, gradePoint6;
public double courseGP1, courseGP2, courseGP3, courseGP4, courseGP5, courseGP6;
int credHour1 = 0, credHour2 = 0, credHour3 = 0, credHour4 = 0, credHour5 = 0, credHour6 = 0;Declaring multiple single variables is bad. There are better ways to represent your data in C#, for example look at using
struct to structure your data entries and an Array or List to hold them for processing in place of defining each variable individually.The variable names are not very meaningful. When this happens it should be taken as a sign that you may have started writing code before you've fully understood the problem.
public Form1()Shows us there is a mix of 'business logic' (or the rules of what need to be processed and how) and 'user interface' code. While it was once common practice in winforms code to do this, it's a bad habit that you shouldn't get into. Any code related to the processing of data should be moved out of this class into a separate file and class.
Moving the business logic to another class also means that you can test your code more easily and can write automated tests that ensure any changes later don't break something that previously worked.
For example you can create a console app which feeds the class known good (and bad) data to make sure that you get the results you expect.
this.Font = new Font("Arial", 15);Is in the wrong place as you can set the default font & size for a form via the forms designer.
private void btnDisplay_Click(object sender, EventArgs e)
{
// to disable certain text boxes according to the number of courses taken
if (rad1.Checked)
...The naming of this code makes me think that after selecting a radio button that users need to click a button to change the interface to allow them to enter the correct data. Check the radio button events and you'll find that they too have a 'click' event which can enable changes to the UI as you select an option.
Radio buttons in general are not especially useful in this context since users can just leave rows blank if they are not needed.
rad1 is not an especially useful name. It doesn't tell someone reviewing the code what the radio button is for.lblCH1.Text = "0";
lblCH2.Text = "0";
lblCH3.Text = "0";Is a situation where you could look at using something called a UserControl and a Control Array; these would allow you to stop repeating large chunks of code with minor changes as you are now.
In fact, comparing
DisplayOne() and DisplayFive() reveals a UI bug. If you run DisplayFive() then we hit the line of code txtCCode6.Hide(); which removes the box entirely but since there is no txtCCode6.Show(); to restore the textbox, the user will never be able to use it. Take this code...private void EnableFields()
{
txtCCode6.Enabled = rad6.Checked;
txtGrade6.Enabled = txtCCode6.Enabled;
// we need to enable txt5 if txt6 is enabled OR
txtCCode5.Enabled = rad5.Checked || txtCCode6.Enabled;
txtGrade5.Enabled = txtCCode5.Enabled;
...Using code like this we can get rid of all the
DisplayOne, DisplayTwo functions and their repeated code and combine them all into one. Whenever the number of entries is set, only one set of code is run and they can't get out of sync.private double FindGradePoint(string grade)
{
double gradePt=0;
switch (grade)
{
case "A+":
case "A":
...Could be shortened extensively using either a
Dictionary or using an enum with a value and the Description attribute.There is quite a bit more that you could do, but what's here will be plenty for now...
Code Snippets
public double gradePoint1, gradePoint2, gradePoint3, gradePoint4, gradePoint5, gradePoint6;
public double courseGP1, courseGP2, courseGP3, courseGP4, courseGP5, courseGP6;
int credHour1 = 0, credHour2 = 0, credHour3 = 0, credHour4 = 0, credHour5 = 0, credHour6 = 0;public Form1()this.Font = new Font("Arial", 15);private void btnDisplay_Click(object sender, EventArgs e)
{
// to disable certain text boxes according to the number of courses taken
if (rad1.Checked)
...lblCH1.Text = "0";
lblCH2.Text = "0";
lblCH3.Text = "0";Context
StackExchange Code Review Q#121647, answer score: 4
Revisions (0)
No revisions yet.