Selecting employees and updating a DataGridView

Question

I've been reading up on different design patterns, mainly MVP and different adaptions of this design pattern / architecture.

Now, I've decided to have a little play with my own ideas, the main outcome separating the user view, from the actual modelling which does all the data changes.

The example is simple, the user is given three radio buttons to select from

• Employee One
• Employee Two
• Employee Three

When these radio buttons are selected it changes a datagrid view with three columns

• Firstname
• Lastname
• Age

My form code has three event driven functions, radioButton_checkChanged.

To start with, I programatically make some new employees: (Ignore the spelling mistake!)

string[] employeeArray = new string[3];
employeeData emplyeeOne = new employeeData("Bob", "Smith", "21");
employeeData emplyeeTwo = new employeeData("John", "Brown", "56");
employeeData emplyeeThree = new employeeData("Andy", "Guy", "28");

 private void employeeOne_CheckedChanged(object sender, EventArgs e)
        {
            updateEmployee(emplyeeOne);
            dataGridView1.Rows.Add(employeeArray);
        }

        private void employeeTwo_CheckedChanged(object sender, EventArgs e)
        {
            updateEmployee(emplyeeTwo);
            dataGridView1.Rows.Add(employeeArray);
        }

        private void employeeThree_CheckedChanged(object sender, EventArgs e)
        {
            updateEmployee(emplyeeThree);
            dataGridView1.Rows.Add(employeeArray);
        }

employeeData class (I've tried to go for encapsulation on this):

public class employeeData
    {
        private string classFirstName { get; set; }
        private string classLastName { get; set; }
        private string classAge { get; set; }

        public employeeData(string firstname, string lastname, string age)
        {
            classFirstName = firstname;
            classLastName = lastname;
            classAge = age;
        }

        public string[] updatePerson()
        {
            string[] data = new string[3];
            data[0] = classFirstName;
            data[1] = classLastName;
            data[2] = classAge;
            return data;
        }
    }

updateEmployee function as seen earlier:

 private void updateEmployee(employeeData who)
    {
        dataGridView1.Rows.Clear();
        dataGridView1.Refresh();
        employeeArray = who.updatePerson();            
    }

Am I on to the beginning of any major software design patterns here? Any adaptations and or improvements you can see to make my code a more robust?

Answer 1

Naming

Based on the naming guidlines

1. classes should be named using PascalCasing casing.
2. methods should be named using PascalCasing casing.
3. properties should be named using PascalCasing casing.

The class prefix does not add any value to the names. Just remove it.

eventhandlers

If these are really radio buttons, the checkedchanged event would be fireing for each radiobutton (if they are placed on the same control ( I assume your form )) because the nature of a radiobutton is that it is unchecked if another one is checked.

So you would need to check if the radiobutton is checked. If this is not the case no update should be done.

private void employeeOne_CheckedChanged(object sender, EventArgs e)
{
    RadioButton button = sender as RadioButton;
    if (button != null && !button.Checked) { return ;}

}  

You should then add a method which updates the view (here gridview). Right now you are doing this at 2 places. First in the eventhandler and then in the updateEmployee() method.

You should create a method which takes two parameters, the emploeyee to add and the datagridview where the employee should be added to.

private void UpdateView(DataGridView view, employeeData who)
{
    string[] employeeArray = who.updatePerson();
    view.Rows.Clear();
    view.Refresh();
    view..Rows.Add(employeeArray);
}

This will be called here

private void employeeOne_CheckedChanged(object sender, EventArgs e)
{
    RadioButton button = sender as RadioButton;
    if (button != null && !button.Checked) { return ;}
    UpdateView(dataGridView1, emplyeeOne);
} 

In this way you can get rid of the classvariable string[] employeeArray.

You should always declare objects as nearest where you need them as possible.

employeeData

The methodname updatePerson does not really reflect what the method do. A better name would be GetAsStringArray[].

The properties are a nice to have, but as they are private you could replace them by local variables.

Age is usually a number and not a string, so you should change the datatype to int. As you need for the updatePerson method the age also, you can just call ToString() on it.

Answer 2

• Naming is important, C# uses Pascal casing for classes and methods, so you should use EmployeeData instead of employeeData, and UpdatePerson instead of updatePerson

• Fields live in classes and thus you don't need to give them a class prefix

  private string FirstName { get; set; }
  private string LastName { get; set; }
  private string Age { get; set; }
  

• C# has a type system that you can use instead of declaring everything as a string

  private int Age {get;set;}
  

• updatePerson uses the verb update, and methods with verbs usually do/change things with side-effects but your method just converts the object to a string[], and to be honest, I don't see any need for this method.

Answer 3

Instead of class employeeData, I suggest using class Employee. Class can encapsulate both data and behavior.

C# convention is to use Pascal casing for class, method and property names.

Here's another take on the code you shared. Note use of private setters, numeric age property and generic List class.

public class Employee
{
    string FirstName { get; private set; }
    string LastName { get; private set; }
    int Age { get; private set; }
    List<string> DisplayInfo { 
        get { return new List<string>() {FirstName, LastName, Age.ToString()}; } 
    }

    public Employee(string first, string last, int age)
    {
        FirstName = first;
        LastName = last;
        Age = age;
    }
}

Answer 4

Your EmployeeData class could benefit from being immutable. I would also rename the updatePerson method to ToArray (as that's what it does) and it also simplifies a lot:

public class EmployeeData
{
    private readonly string firstName;

    private readonly string lastName;

    private readonly string age;

    public EmployeeData(string firstName, string lastName, string age)
    {
        this.firstName = firstName;
        this.lastName = lastName;
        this.age = age;
    }

    public string[] ToArray()
    {
        return new[] { this.firstName, this.lastName, this.age };
    }
}