Saving student scores

Question

I have a program where it takes the students name and last name and score and saves it. When checking to see if a textbox has information in it, I use a lot of if statements. Is there a cleaner way to do this?

public void CheckData()
 {
 if (first1.Text != empty & score1.Text != empty & last1.Text != empty &
 first2.Text != empty & score2.Text != empty & last2.Text != empty &
 first3.Text != empty & score3.Text != empty & last3.Text != empty &
 first4.Text != empty & score4.Text != empty & last4.Text != empty &
 first5.Text != empty & score5.Text != empty & last5.Text != empty)
 {
 student1 = new Student(first1.Text, last1.Text, Convert.ToInt32(score1.Text));
 student2 = new Student(first2.Text, last2.Text, Convert.ToInt32(score2.Text));
 student3 = new Student(first3.Text, last3.Text, Convert.ToInt32(score3.Text));
 student4 = new Student(first4.Text, last4.Text, Convert.ToInt32(score4.Text));
 student5 = new Student(first5.Text, last5.Text, Convert.ToInt32(score5.Text));
 if (first6.Text != empty & score6.Text != empty & last6.Text != empty)
 {
 student6 = new Student(first6.Text, last6.Text, Convert.ToInt32(score6.Text));
 if (first7.Text != empty & score7.Text != empty & last7.Text != empty)
 {
 student7 = new Student(first7.Text, last7.Text, Convert.ToInt32(score7.Text));
 if (first8.Text != empty & score8.Text != empty & last8.Text != empty)
 {
 student8 = new Student(first8.Text, last8.Text, Convert.ToInt32(score8.Text));
 if (first9.Text != empty & score9.Text != empty & last9.Text != empty)
 {
 student9 = new Student(first9.Text, last9.Text, Convert.ToInt32(score9.Text));
 if (first10.Text != empty & score10.Text != empty & last10.Text != empty)
 {
 student10 = new Student(first10.Text, last10.Text, Convert.ToInt32(score10.Text));
 if (first11.Text != empty & score11.Text != empty & last11.Text != empty)
 {
 student11 = new Student(first11.Text, last11.Text, Convert.ToInt32(score11.Text));
 if (first12.Text != empty & score12.Text != empty & last12.Text != empty)
 {
 student12 = new Student(first12.Text, last12.Text, Convert.ToInt32(score12.Text));
 if (first13.Text != empty & score13.Text != empty & last13.Text != empty)
 {
 student13 = new Student(first13.Text, last13.Text, Convert.ToInt32(score13.Text));
 if (first14.Text != empty & score14.Text != empty & last14.Text != empty)
 {
 student14 = new Student(first14.Text, last14.Text, Convert.ToInt32(score14.Text));
 if (first15.Text != empty & score15.Text != empty & last15.Text != empty)
 {
 student15 = new Student(first15.Text, last15.Text, Convert.ToInt32(score15.Text));
 }
 }
 }
 }
 }
 }
 }
 }
 }
 }
 }
 else
 {
 MessageBox.Show("You need a Minimum of 5 Students");
 }
 }

Variables

Student student1;
Student student2;
Student student3;
Student student4;
Student student5;
Student student6;
Student student7;
Student student8;
Student student9;
Student student10;
Student student11;
Student student12;
Student student13;
Student student14;
Student student15;
string empty = "";
public class Student
{
 string First { get; set; }
 string Last { get; set; }
 int Score { get; set; }
 public Student(string first, string last, int score)
 {
 first = First;
 last = Last;
 score = Score;
 }
}

• svick: You should have the textboxes and the students in a collection, not have fifteen variables.
• Leonid: Yikes! You certainly need to use right data structure(s) here to make your life simple. Also consider using a DataGridView instead of 32 TextBoxes, since you are working with a table essentially.
• Liam McInroy: @Leonid Lol 45 Textboxes
• Liam McInroy: @svick Like a Dictionary?
• Leonid: 45 is an odd number ... so, not everything logically belongs in one table obviously. I have found it very useful to use separate panes - one for buttons at the bottom, one for the grid in the middle, and one for other inputs and controls at the top. Make sure to experiment with the Dock property for these panes. Since Winforms, unlike WPF are tied to a specific resolution, and can look like crap on a different screen (and maybe there is a better approach), I find it is much easier to implement the resizing logic when I use separate panes and docking. As far as collections, List<...> should do.

Answer 1

The duplication problem can be solved using a custom UserControl, to encapsulate the controls used to edit a Student, as well as the validation.

To define the control:

• right click you project in the Solution Explorer
• select Add -> UserControl...
• enter a name for the control (ex: StudentControl)
• press the Add button

After adding the user control, you can design it just like you would any other form. Add all the controls needed for a single Student: the three textboxes (for example, txtFirstName, txtLastName and txtScore), as well as labels, if you consider necessary.

Build your solution.

After that, on your form, instead of the 45 textboxes, place 15 StudentControls (you should find them in the Toolbox).

Now, for the code part.

Your control needs to be able to read a student from its controls, so inside the StudentControl class, add the following code:

public Student GetStudent()
{
 if (txtFirstName.Text == String.Empty || 
 txtLastName.Text == String.Empty || 
 txtScore.Text == String.Empty)
 return null;
 return new Student(txtFirstName.Text, txtLastName.Text, int.Parse(txtScore.Text));
}

Now your CheckData() method can be simplified like this:

public void CheckData()
{
 var students = Controls.OfType<StudentControl>()
 .Select(studentControl => studentControl.GetStudent())
 .Where(student => student != null)
 .ToList();
 if(students.Count<5)
 MessageBox.Show("You need a Minimum of 5 Students");
}

Doing this we've gained some advantages:

• we've reduced the duplication. If some day you have to add a new field for a Student, you only have to add it once, not 15 times. Also, if tomorrow you need you form to hold 10 or 20 students, that's a simple operation to perform in the form designer, with no code implications.
• we've separated concerns: now the user control handles the logic of editing a single student, while the form only knows that it has some StudentControl instances that are used to somewhow construct Student objects.
• the code got significantly smaller. This means that anyone can read and understand it faster, thus leading to better maintainability.

• Bhupendra: Yeah validation is very good way to separate the concerns. For wpf look at IDataErrorInfo that will prevent you from doing those sorts of checks and keep the user informed. "Prevention is better than cure"

Answer 2

First things first, that nesting is horrible! You could revert the conditions on the ifs, and use a return to avoid using an else.

public void BadNesting() {
 if(cond1) {
 //...
 if(cond2) {
 // ...
 if(cond3) {
 //...
 }
 }
 }
}
public void Better() {
 if(!cond1) {
 return;
 }
 // ...
 if(!cond2) {
 return;
 }
 // ...
 if(!cond3) {
 return;
 }
 // ...
}

You also repeat your code a lot. We can use the suggestions to use a DataGridView and make the code magically expand to however many students you might get:

List<Student> Students;
public void CheckData(int minStudents = 5) {
 Students = new List<Student>();
 for (int i = 0; i < gridView.Rows.Count; i++) {
 var row = gridView.Rows[i];
 TextBox firstName = (TextBox)row.Controls[0];
 TextBox lastName = (TextBox)row.Controls[1];
 TextBox scoreTB = (TextBox)row.Controls[2];
 string name = firstName.Text;
 string familyName = lastName.Text;
 int score;
 if (string.IsNullOrEmpty(name)
 || string.IsNullOrEmpty(familyName)
 || !int.TryParse(scoreTB.Text, out score)
 ) {
 // Will not parse any more students.
 if (i < minStudents) {
 MessageBox.Show(string.Format("You need a minimum of {0} students", minStudents));
 }
 break;
 }
 var student = new Student(name, familyName, score);
 Students.Add(student);
 }
}

• Liam McInroy: Um, this is wpf, not winforms. No such thing as DataGridView
• ANeves: Hehe, and now you get why my answer is community wiki. ;) But surely there is an equivalent control? Anyway, I would prefer @w0lf's answer.
• Liam McInroy: Yes, but I my UserControl does not show up on wpf:P