14
\$\begingroup\$

I started out with the best of intentions, but this form got hacky real fast.

It's purpose is to serve as a create new Student form. Also, if you want to view an existing Students information.

Think of it as the CRU of CRUD.

Here it is:

using System; using System.Collections.Generic; using System.ComponentModel; using System.Data; using System.Drawing; using System.Linq; using System.Text; using System.Windows.Forms; using Tutomentor.Branding; using Tutomentor.Data.Repositories; namespace Tutomentor.UI.Students { public partial class StudentInformation : Form { StudentRepository repo = new StudentRepository(); bool IsCreating = false; Student student; public StudentInformation() { InitializeComponent(); LoadComboBoxes(); LoadBranding(); IsCreating = true; } public StudentInformation(int studentID) { InitializeComponent(); LoadComboBoxes(); LoadBranding(); student = repo.FindStudent(studentID); LoadStudentInformation(student); } private void LoadComboBoxes() { cmbGrade.DisplayMember = "Name"; cmbGrade.ValueMember = "ID"; cmbGradeParalelo.DisplayMember = "Name"; cmbGradeParalelo.ValueMember = "ID"; GradeRepository repo = new GradeRepository(); cmbGrade.DataSource = repo.FindAllGrades(); } private void LoadStudentInformation(Student student) { cmbGrade.SelectedValue = student.GradeParalelo.Grade.ID; cmbGradeParalelo.SelectedValue = student.IDGrade; txtRude.Text = student.RUDE.ToString(); txtNombrePadre.Text = student.FatherName; txtProfesionPadre.Text = student.FatherProfession; txtCelularPadre.Text = student.MobilePhoneFather; txtLugarDeTrabajoPadre.Text = student.PlaceofWorkFather; txtNombreMadre.Text = student.MotherName; txtProfesionMadre.Text = student.MotherProfession; txtCelularMadre.Text = student.MobilePhoneMother; txtLugarDeTrabajoMadre.Text = student.PlaceofWorkMother; txtObservaciones.Text = student.Observations; txtNombre.Text = student.Name; txtApellidoPaterno.Text = student.FatherLastName; txtApellidoMaterno.Text = student.MotherLasteName; dtpFechaNacimiento.Value = Convert.ToDateTime(student.DateOfBirth); txtLugarNacimiento.Text = student.PlaceOfBirth; SetSex(student.Sex); txtCarnet.Text = student.Carnet; txtTelefono.Text = student.Telephone; txtCelular.Text = student.MobilePhone; txtDireccion.Text = student.Address; } private void SetSex(string p) { if (p == "M") { sexoMasculino.Checked = true; sexoFemenino.Checked = false; } else { sexoMasculino.Checked = false; sexoFemenino.Checked = true; } } private void LoadBranding() { this.Text = "Tutomentor - Agregando nuevo alumnos."; this.BackColor = Brand.PrimaryColor; panelBorderLeft.BackColor = Brand.HeaderColor; panelBorderRight.BackColor = Brand.HeaderColor; panelSeparator1.BackColor = Brand.SeparatorColor; } private void cmbGrade_SelectedIndexChanged(object sender, EventArgs e) { LoadGradeParalelos(); } private void LoadGradeParalelos() { GradeParaleloRepository repo = new GradeParaleloRepository(); Int64 gradeID = Convert.ToInt64(cmbGrade.SelectedValue); cmbGradeParalelo.DataSource = repo.FindAllGradeParalelos().Where(g => g.IDGrade == gradeID); } private void btnSave_Click(object sender, EventArgs e) { SaveInformation(); } private void SaveInformation() { if (IsCreating) { Student newStudent = new Student(); Int64 gradeId = Convert.ToInt64(cmbGradeParalelo.SelectedValue); newStudent.IDGrade = gradeId; newStudent.RUDE = Convert.ToInt64(txtRude.Text); /*Parents information.*/ newStudent.FatherName = txtNombrePadre.Text; newStudent.FatherProfession = txtProfesionPadre.Text; newStudent.MobilePhoneFather = FormatPhoneNumber(txtCelularPadre.Text); newStudent.PlaceofWorkFather = txtLugarDeTrabajoPadre.Text; newStudent.MotherName = txtNombreMadre.Text; newStudent.MotherProfession = txtProfesionMadre.Text; newStudent.MobilePhoneMother = FormatPhoneNumber(txtCelularMadre.Text); newStudent.PlaceofWorkMother = txtLugarDeTrabajoMadre.Text; /*newStudent information*/ newStudent.Name = txtNombre.Text; newStudent.FatherLastName = txtApellidoPaterno.Text; newStudent.MotherLasteName = txtApellidoMaterno.Text; newStudent.DateOfBirth = dtpFechaNacimiento.Value.ToShortDateString(); newStudent.PlaceOfBirth = txtLugarNacimiento.Text; newStudent.Sex = sexoMasculino.Checked ? sexoMasculino.Text : sexoFemenino.Text; newStudent.Telephone = FormatPhoneNumber(txtTelefono.Text); newStudent.MobilePhone = FormatPhoneNumber(txtCelular.Text); newStudent.Address = txtDireccion.Text; newStudent.Carnet = FormatPhoneNumber(txtCarnet.Text); newStudent.Observations = txtObservaciones.Text; repo.Add(newStudent); repo.Save(); MessageBox.Show("Se guardo el registro exitosamente.", "Exito!", MessageBoxButtons.OK, MessageBoxIcon.Information, MessageBoxDefaultButton.Button1); ClearForm(); } else { Int64 gradeId = Convert.ToInt64(cmbGradeParalelo.SelectedValue); student.IDGrade = gradeId; student.RUDE = Convert.ToInt64(txtRude.Text); /*Parents information.*/ student.FatherName = txtNombrePadre.Text; student.FatherProfession = txtProfesionPadre.Text; student.MobilePhoneFather = FormatPhoneNumber(txtCelularPadre.Text); student.PlaceofWorkFather = txtLugarDeTrabajoPadre.Text; student.MotherName = txtNombreMadre.Text; student.MotherProfession = txtProfesionMadre.Text; student.MobilePhoneMother = FormatPhoneNumber(txtCelularMadre.Text); student.PlaceofWorkMother = txtLugarDeTrabajoMadre.Text; /*student information*/ student.Name = txtNombre.Text; student.FatherLastName = txtApellidoPaterno.Text; student.MotherLasteName = txtApellidoMaterno.Text; student.DateOfBirth = dtpFechaNacimiento.Value.ToShortDateString(); student.PlaceOfBirth = txtLugarNacimiento.Text; student.Sex = sexoMasculino.Checked ? sexoMasculino.Text : sexoFemenino.Text; student.Telephone = FormatPhoneNumber(txtTelefono.Text); student.MobilePhone = FormatPhoneNumber(txtCelular.Text); student.Address = txtDireccion.Text; student.Carnet = FormatPhoneNumber(txtCarnet.Text); student.Observations = txtObservaciones.Text; repo.Save(); MessageBox.Show("Se guardo el registro exitosamente.", "Exito!", MessageBoxButtons.OK, MessageBoxIcon.Information, MessageBoxDefaultButton.Button1); this.Close(); } } private string FormatPhoneNumber(string p) { return p.Insert(3, "-"); } private void btnClear_Click(object sender, EventArgs e) { ClearForm(); } private void ClearForm() { Action<Control.ControlCollection> func = null; func = (controls) => { foreach (Control control in controls) if (control is TextBox) (control as TextBox).Clear(); else func(control.Controls); }; func(Controls); } } }

I don't know how to improve this class because every line of code is needed.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I like to line up the equal signs. It makes the code easier to read for me. \$\endgroup\$ Commented Jan 28, 2011 at 1:16
  • \$\begingroup\$ You don't need to pass student to LoadStudentInformation if you're storing it in a class variable. \$\endgroup\$ Commented Jan 28, 2011 at 9:38

6 Answers 6

Reset to default
10
\$\begingroup\$

You can simplify SaveInformation:

private void SaveInformation() { if (IsCreating) { Student newStudent = new Student(); repo.Add(newStudent); } Int64 gradeId = Convert.ToInt64(cmbGradeParalelo.SelectedValue); newStudent.IDGrade = gradeId; newStudent.RUDE = Convert.ToInt64(txtRude.Text); /*Parents information.*/ newStudent.FatherName = txtNombrePadre.Text; newStudent.FatherProfession = txtProfesionPadre.Text; newStudent.MobilePhoneFather = FormatPhoneNumber(txtCelularPadre.Text); newStudent.PlaceofWorkFather = txtLugarDeTrabajoPadre.Text; newStudent.MotherName = txtNombreMadre.Text; newStudent.MotherProfession = txtProfesionMadre.Text; newStudent.MobilePhoneMother = FormatPhoneNumber(txtCelularMadre.Text); newStudent.PlaceofWorkMother = txtLugarDeTrabajoMadre.Text; /*newStudent information*/ newStudent.Name = txtNombre.Text; newStudent.FatherLastName = txtApellidoPaterno.Text; newStudent.MotherLasteName = txtApellidoMaterno.Text; newStudent.DateOfBirth = dtpFechaNacimiento.Value.ToShortDateString(); newStudent.PlaceOfBirth = txtLugarNacimiento.Text; newStudent.Sex = sexoMasculino.Checked ? sexoMasculino.Text : sexoFemenino.Text; newStudent.Telephone = FormatPhoneNumber(txtTelefono.Text); newStudent.MobilePhone = FormatPhoneNumber(txtCelular.Text); newStudent.Address = txtDireccion.Text; newStudent.Carnet = FormatPhoneNumber(txtCarnet.Text); newStudent.Observations = txtObservaciones.Text; repo.Save(); MessageBox.Show("Se guardo el registro exitosamente.", "Exito!", MessageBoxButtons.OK, MessageBoxIcon.Information, MessageBoxDefaultButton.Button1); if (IsCreating) { ClearForm(); } else { this.Close(); } }

The second one is a style thing. Some people like this others hate it. 

 private void SetSex(string p) { sexoMasculino.Checked = (p == "M"); sexoFemenino.Checked = !sexoMasculino.Checked; }

Additional note: It is unclear from the given text if p will have already been sanitized. As a result (and depending on expected usage) you want to also check for "m" (if this is a possibility).

\$\endgroup\$
5
  • \$\begingroup\$ I see no reason to hate the Boolean thing unless you really don't understand Boolean logic. Besides, it reads nicely: sexoMasculino.Checked gets whether p equals "M". \$\endgroup\$ Commented Jan 28, 2011 at 2:41
  • \$\begingroup\$ I like the first line of the SetSex function: the = (p == "M") part. I would have used an If-statement. I think I will learn many useful coding idioms on this site. I wonder if it works in other languages. \$\endgroup\$ Commented Jan 28, 2011 at 8:39
  • \$\begingroup\$ There is a problem with the case, though. What if the sex was passed as "m"? You must compare both cases or normalize first. a || between them might be reasonable. However it seems calling Equals is more efficient because the == operator calls that function in the end, so why not cut the middle man? \$\endgroup\$ Commented Jan 28, 2011 at 12:09
  • 1
    \$\begingroup\$ @Voulnet: Thinking of efficiency here is a macro optimization. Until we can show that this is an efficiency bottleneck clarity is the only goal. Weather == or Equal is more clear is style. I prefer ==. \$\endgroup\$ Commented Jan 28, 2011 at 12:49
  • \$\begingroup\$ @LokiAstari Does visual studio offer any way to automatically align equals signs (or protect them during automatic reformatting)? I used to align mine back when all layout/alignment was manual; but stopped because the value of being able to fix idents/wrap long lines automatically was more beneficial than any manually inserted whitespace to align code into columns. \$\endgroup\$ Commented Nov 14, 2012 at 21:55
6
\$\begingroup\$

Sorry folks, I cannot comment on anyone's reply because of my points. But I would refactor Martin York's SetSex() method to look like this: 

private void SetSex(string p) { sexoMasculino.Checked = p.Equals("m", StringComparison.OrdinalIgnoreCase); sexoFemenino.Checked = !sexoMasculino.Checked; }
\$\endgroup\$
4
  • 1
    \$\begingroup\$ You would only redactor my version? Not the original version? :-( \$\endgroup\$ Commented Jan 28, 2011 at 7:41
  • 1
    \$\begingroup\$ @cod3-monk3y, how about ` = ((p == "m") || (p == "M"))`? Is it not more efficient than the method call? \$\endgroup\$ Commented Jan 28, 2011 at 11:12
  • \$\begingroup\$ @Martin York You already refactored the original version, so you should take the credit for that. I wanted to comment but couldn't because of my points. @Geoffrey I don't know how efficient it is, but it looks more elegant and is more readable. If I've to be so much concerned about efficiency that I've to start thinking about calling function, I would better use assembly and not a high level language. \$\endgroup\$ Commented Jan 28, 2011 at 16:43
  • 1
    \$\begingroup\$ Whether or not it needs to be a case insensitive comparison depends upon how the incoming value is set, and what it means. Perhaps "m" should result in false, or more likely, perhaps it's impossible. \$\endgroup\$ Commented Mar 26, 2012 at 2:17
4
\$\begingroup\$

Look at your code within this block inside the SaveInformation method.

if (IsCreating) { // code here } else { // and here }

Start refactoring with this. How many lines of code are actually different here? Outside of a different object name (newStudent vs. student), there are not very many. Looks to me like you can create a method with the Student object as a parameter so that you can populate each of the relevant attributes.

\$\endgroup\$
3
\$\begingroup\$ 
public StudentInformation() { InitializeComponent(); LoadComboBoxes(); LoadBranding(); IsCreating = true; } public StudentInformation(int studentID) { InitializeComponent(); LoadComboBoxes(); LoadBranding(); student = repo.FindStudent(studentID); LoadStudentInformation(student); }

can be refactored to,

// default IsCreating to true bool IsCreating = true; public StudentInformation() { InitializeComponent(); LoadComboBoxes(); LoadBranding(); } public StudentInformation(int studentID) : this() { IsCreating = false; student = repo.FindStudent(studentID); LoadStudentInformation(student); }

Also there is lot of code in your Form control class which should be part of controller (MVC) / presenter (MVP) like using those repositories also save information should be with controller part to notify the model to save data.

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

First of all you have to use separate class for Business Logic (to save information) and then you should use that class object and add ALL text values as parameters in SaveInformation function rather than using textbox value directly.

\$\endgroup\$
2
\$\begingroup\$

One could also argue that you're mixing a lot of presentation code with business logic (creation/modification of data). Don't do that. Separate presentation from content.

Have the form call a ViewModel/Controller style of object that handles saving student information and contains the repository. Then the form code can focus purely on presentational elements, and the other object can handle validation, modification, etc. Separation of concerns is good, young padawan.

\$\endgroup\$

You must log in to answer this question.