current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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?

share|improve this question
    
Is this ASP or WPF? –  ANeves Nov 7 '14 at 13:56
    
.Net 4.5 Window Forms. –  Johnathan Brown Nov 7 '14 at 15:37
1  
Could you please use a different title for the question. State what your code does. –  JaDogg Nov 7 '14 at 15:47
    
I have stated what my code does, in the question haha ;) –  Johnathan Brown Nov 7 '14 at 15:58

4 Answers 4

up vote 3 down vote accepted

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.

share|improve this answer
    
Literally learnt so much from your reply, thank you for that! I haven't been using C# for that long - came over from C++ - so things are a little different! –  Johnathan Brown Nov 7 '14 at 12:25
up vote 3 down vote
  • 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.

share|improve this answer
    
Thank you for the reply, I will defiantly remember PascalCasing. A question about that the type system, does the program calculate what type is needed at runtime or compile time? –  Johnathan Brown Nov 7 '14 at 12:26
    
@JohnathanBrown Compile time, unless the type is dynamic then it is determined at runtime msdn.microsoft.com/en-GB/library/dd264736.aspx –  Sleiman Jneidi Nov 7 '14 at 12:34
    
Okay cracking, thanks. –  Johnathan Brown Nov 7 '14 at 15:38
up vote 1 down vote

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;
    }
}
share|improve this answer
    
I quite like the modifications you have done here, and I also really like the private setters. I most defiantly need think more about how I name things! Thank you –  Johnathan Brown Nov 10 '14 at 10:21
up vote 1 down vote

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 };
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.