Nsryjdtyk

I'm looking for some feedback on my code here. I want to eliminate duplicating the code through the different button_clicks. I'm thinking with a method but nothing I try works better than what I have.

using System;

using System.Collections.Generic;

using System.Data;

using System.IO;

using System.Linq;

using System.Windows.Forms;

using Newtonsoft.Json;

using Newtonsoft.Json.Linq;



namespace EmployeePayDataWk4

{

public partial class Employee_Pay_Form : Form

{       



    public Employee_Pay_Form()

    {

        InitializeComponent();            

    }



    private void Employee_Pay_Form_Load(object sender, EventArgs e)

    {

        EmployeeDataGridView.ColumnCount = 8;

        EmployeeDataGridView.Columns[0].Name = "Employee Name";

        EmployeeDataGridView.Columns[1].Name = "Zip Code";

        EmployeeDataGridView.Columns[2].Name = "Age";

        EmployeeDataGridView.Columns[3].Name = "Monthly Gross Pay";

        EmployeeDataGridView.Columns[4].Name = "Department ID";

        EmployeeDataGridView.Columns[5].Name = "Developer Type";

        EmployeeDataGridView.Columns[6].Name = "Annual Taxes";

        EmployeeDataGridView.Columns[7].Name = "Annual Net Pay";            



    }



    private void LoadAllButton_Click(object sender, EventArgs e)

    {

        EmployeeDataGridView.Rows.Clear();

        //Read from JSON file

        string JSONstring = File.ReadAllText("JSON.json");

        List<Employee> employees = JsonConvert.DeserializeObject<List<Employee>>(JSONstring);



        //Display into DataGridView

        foreach (Employee emp in employees)

        {

            string row = { emp.Name, emp.Zip, emp.Age.ToString(), string.Format("{0:C}", emp.Pay),

                emp.DepartmentId.ToString(), SetDevType(emp.DepartmentId),

                string.Format("{0:C}", emp.CalculateTax(emp.Pay)),

                string.Format("{0:C}", AnnualPay(emp.Pay) - emp.CalculateTax(emp.Pay))};

            EmployeeDataGridView.Rows.Add(row);

        }

    }







    private void FTEmployeeButton_Click(object sender, EventArgs e)

    {

        EmployeeDataGridView.Rows.Clear();



        //Read from JSON file

        string JSONstring = File.ReadAllText("JSON.json");

        List<Employee> employees = JsonConvert.DeserializeObject<List<Employee>>(JSONstring);



        //LINQ Query for FT Employees

        var FTEmp = from emp in employees

                    where emp.GetTaxForm == "W2"

                    select emp;



        //Display into DataGridView

        foreach (Employee emp in FTEmp)

        {

            string row = { emp.Name, emp.Zip, emp.Age.ToString(), string.Format("{0:C}", emp.Pay),

                emp.DepartmentId.ToString(), SetDevType(emp.DepartmentId),

                string.Format("{0:C}", emp.CalculateTax(emp.Pay)),

                string.Format("{0:C}", AnnualPay(emp.Pay) - emp.CalculateTax(emp.Pay))};

            EmployeeDataGridView.Rows.Add(row);

        }

    }



    private void ContractEmployeeButton_Click(object sender, EventArgs e)

    {

        EmployeeDataGridView.Rows.Clear();



        //Read from JSON file

        string JSONstring = File.ReadAllText("JSON.json");

        List<Employee> employees = JsonConvert.DeserializeObject<List<Employee>>(JSONstring);



        //LINQ Query for Contract Employees

        var contractEmp = from emp in employees

                          where emp.GetTaxForm == "1099"

                          select emp;



        //Display into DataGridView

        foreach (Employee emp in contractEmp)

        {

            string row = { emp.Name, emp.Zip, emp.Age.ToString(), string.Format("{0:C}", emp.Pay),

                emp.DepartmentId.ToString(), SetDevType(emp.DepartmentId),

                string.Format("{0:C}", emp.CalculateTax(emp.Pay)),

                string.Format("{0:C}", AnnualPay(emp.Pay) - emp.CalculateTax(emp.Pay))};

            EmployeeDataGridView.Rows.Add(row);

        }

    }





    //Method to determine developer type

    string typeName;

    public string SetDevType(int id)

    {

        if (id == 1)

        {

            typeName = "Object-Oriented";

        }

        else if (id == 2)

        {

            typeName = "Scripts";

        }

        else { typeName = "Unknown"; }

        return typeName;

    }



    public double AnnualPay(double amount) => 12 * amount;

}





class Employee : IFilingStatus

{

    public Employee() { }



    public string Name { get; set; }

    public string Zip { get; set; }

    public int Age { get; set; }

    public double Pay { get; set; }

    public int DepartmentId { get; set; }  

    public string GetTaxForm { get; set; }



    public double CalculateTax(double basis)

    {

        double monthlyTax; 



        if ((GetTaxForm == "W2") || (GetTaxForm == "w2"))

        {

            monthlyTax = .07 * basis;

        }

        else

        {

            monthlyTax = 0;

        }

        return 12 * monthlyTax;

    }

    public double AnnualPay(double amount) => 12 * amount;

}



public interface IFilingStatus

{

    double CalculateTax(double basis);

}



}

Here's the JSON file

[



 {

"Name": "Jeff",

"Zip": "55422",

"Age": 54,

"Pay": 9587.23,

"DepartmentId": 1,

"GetTaxForm": "1099"

},



{

"Name": "Dave",

"Zip": "03456",

"Age": 41,

"Pay": 8547.55,

"DepartmentId": 1,

"GetTaxForm": "W2"

},



{

"Name": "Amber",

"Zip": "41908",

"Age": 35,

"Pay": 4878.1,

"DepartmentId": 2,

"GetTaxForm": "W2"

},



{

"Name": "Cassie",

"Zip": "91820",

"Age": 28,

"Pay": 4500,

"DepartmentId": 1,

"GetTaxForm": "1099"

},



{

"Name": "Albert",

"Zip": "54321",

"Age": 39,

"Pay": 5874.09,

"DepartmentId": 2,

"GetTaxForm": "1099"

}

]

Your code is fairly clean and easy to read and follow. But you should definitely think about not repeating yourself. Even just a single or two line - when you feel encouraged to copy/paste - don't! Make a method and call that from where needed.

When you do something like this:

    EmployeeDataGridView.ColumnCount = 8;

    EmployeeDataGridView.Columns[0].Name = "Employee Name";

    EmployeeDataGridView.Columns[1].Name = "Zip Code";

    EmployeeDataGridView.Columns[2].Name = "Age";

    EmployeeDataGridView.Columns[3].Name = "Monthly Gross Pay";

    EmployeeDataGridView.Columns[4].Name = "Department ID";

    EmployeeDataGridView.Columns[5].Name = "Developer Type";

    EmployeeDataGridView.Columns[6].Name = "Annual Taxes";

    EmployeeDataGridView.Columns[7].Name = "Annual Net Pay";   

there is obviously better ways that is easier to maintain - an array and a loop for instance:

  DataGridView employeeDataGridView = EmployeeDataGridView;



  string headers =         

  {

    "Employee Name",

    "Zip Code",

    "Age",

    "Monthly Gross Pay",

    "Department ID",

    "Developer Type",

    "Annual Taxes",

    "Annual Net Pay",

  };



  employeeDataGridView.ColumnCount = headers.Length;



  for (int i = 0; i < headers.Length; i++)

  {

    employeeDataGridView.Columns[i].Name = headers[i];

  }

This is easier to maintain. A new column is just inserted in the headers list, and reordering can be done there too - in one place.

//Method to determine developer type

string typeName;

public string SetDevType(int id)

{

  if (id == 1)

  {

    typeName = "Object-Oriented";

  }

  else if (id == 2)

  {

    typeName = "Scripts";

  }

  else { typeName = "Unknown"; }

  return typeName;

}

Here the typeName field is placed outside the method. Why that? And you could use an switch-case statement instead of the if's:

public string SetDevType(int id)

{

  switch (id)

  {

    case 1:

      return "Object-Oriented";

    case 2:

      return "Scripts";

    default:

      return "Unknown";

  }

}

user2156791 shows a good way to refactor the initialization of the grid, but it can be done even "tighter":

private IEnumerable<Employee> LoadEmployees(string filePath)

{

  //Read from JSON file

  string JSONstring = File.ReadAllText(filePath);

  return JsonConvert.DeserializeObject<IEnumerable<Employee>>(JSONstring);

}



private void InitializeGrid(Func<IEnumerable<Employee>, IEnumerable<Employee>> employeeSelector)

{

  try

  {

    EmployeeDataGridView.Rows.Clear();



    IEnumerable<Employee> employees = LoadEmployees(@"JSON.json");

    if (employees == null)

      throw new NullReferenceException("Unable to read from the data source file");



    foreach (Employee employee in employeeSelector(employees))

    {

      string row =

      {

        employee.Name,

        employee.Zip,

        employee.Age.ToString(),

        string.Format("{0:C}", employee.Pay),

        employee.DepartmentId.ToString(),

        SetDevType(employee.DepartmentId),

        string.Format("{0:C}",

        employee.CalculateTax(employee.Pay)),

        string.Format("{0:C}", AnnualPay(employee.Pay) - employee.CalculateTax(employee.Pay))

       };



      EmployeeDataGridView.Rows.Add(row);

    }

  }

  catch (Exception ex)

  {

    MessageBox.Show(ex.Message);

  }

}



private void LoadAllButton_Click(object sender, EventArgs e)

{

  InitializeGrid(employees => employees);

}



private void FTEmployeeButton_Click(object sender, EventArgs e)

{

  InitializeGrid(employees => from emp in employees

                              where emp.GetTaxForm == "W2"

                              select emp);

}



private void ContractEmployeeButton_Click(object sender, EventArgs e)

{

  InitializeGrid(employees => from emp in employees

                              where emp.GetTaxForm == "1099"

                              select emp);

}

Here everything is only done in one place, and it's easy to maintain and extent or change. Because the data source is always the same a selector delegate is provided to InitializeGrid() instead of the entire source.

Futher: when interacting with the user through event handlers you should care about handling exceptions and errors and display appropriate messages to the user. A try-catch around everything is a place to start.

Let's first check what your code is doing repeatedly

• Read the json file and fill a List<Employee>


• Filter this list by checking the GetTaxForm property of the employee which by the way is a bad name for a property, just TaxForm would be better or return all employees


• Display the resulting List<Employee> in a DataGridView

Now let us check what your code isn't doing

• It doesn't change the json file

Improvements

• I suggest reading the json-file only once and fill a List<Employee> which you filter if needed by the desired property.




• Having a method DisplayEmployees() or like @user2156791 stated in his/her answer FillEmployeeDataGrid() (but I would pass an IEnumerable<Employee> as the method argument).

This

//Method to determine developer type

string typeName;

public string SetDevType(int id)

{

    if (id == 1)

    {

        typeName = "Object-Oriented";

    }

    else if (id == 2)

    {

        typeName = "Scripts";

    }

    else { typeName = "Unknown"; }

    return typeName;

}  

looks strange in many ways. The method is called SetXX() but is getting a value. The class level field typeName is only used in this method so why is it a class level field ?

Why do you have public double AnnualPay(double amount) => 12 * amount; inside the Employee_Pay_Form class ? Why don't you use the ´AnnualPay()from theEmployee` class ?

Implementing the mentioned points will lead to

private static List<Employee> LoadEmployees(string fileName)

{

    if (string.IsNullOrWhiteSpace(fileName))

    {

        return new List<Employee>();

    }

    string content = File.ReadAllText("JSON.json");

    return JsonConvert.DeserializeObject<List<Employee>>(content );

}

which is called once at startup and stored in a class-level field List<Employee> eployees.

private void DisplayEmployees(IEnumerable<Employee> avaibleEmployees)

{

    EmployeeDataGridView.Rows.Clear();



    foreach (var employee in avaibleEmployees)

    {

        string row = 

        { 

            employee.Name, 

            employee.Zip, 

            employee.Age.ToString(), 

            string.Format("{0:C}", employee.Pay),

            employee.DepartmentId.ToString(), 

            employee.FetchDevType(employee.DepartmentId),

            string.Format("{0:C}", employee.CalculateTax(emp.Pay)),

            string.Format("{0:C}", employee.AnnualPay(emp.Pay) - employee.CalculateTax(emp.Pay))

        };

        EmployeeDataGridView.Rows.Add(row);

    }

}

where FetchDevType() looks like so

public string FetchDevType(int departmentId)

{

    switch (departmentId)

    {

        case 1:

            return "Object-Oriented";

        case 2:

            return "Scripts";

        default:

            return "Unknown";

    }

}

and should be placed inside the Employee class.

private IEnumerable<Employee> FilterByTaxForm(string desiredTaxForm)

{

    return from employee in employees

                    where employoee.TaxForm == desiredTaxForm

                    select employee;

}  

which is called where you need to filter the eployees like e.g so

private void ContractEmployeeButton_Click(object sender, EventArgs e)

{

    DisplayEmployees(FilterByTaxForm("1099"));

}

Please check below refactoring code.

I have check the code and refactoring code with common method. So, if you have change common method then effect to all place. No need to change every place where you write same code or same logic.

private void LoadAllButton_Click(object sender, EventArgs e)

        {

            //Read from JSON file

            List<Employee> employees = ReadJsonData();



            FillEmployeeDataGrid(employees);

        }



        private List<Employee> ReadJsonData()

        {

            string JSONstring = File.ReadAllText("JSON.json");

            List<Employee> employees = JsonConvert.DeserializeObject<List<Employee>>(JSONstring);

            return employees;

        }



        private void FillEmployeeDataGrid(List<Employee> employees)

        {

            EmployeeDataGridView.Rows.Clear();

            //Display into DataGridView

            foreach (Employee emp in employees)

            {

                string row = { emp.Name, emp.Zip, emp.Age.ToString(), string.Format("{0:C}", emp.Pay),

                emp.DepartmentId.ToString(), SetDevType(emp.DepartmentId),

                string.Format("{0:C}", emp.CalculateTax(emp.Pay)),

                string.Format("{0:C}", AnnualPay(emp.Pay) - emp.CalculateTax(emp.Pay))};

                EmployeeDataGridView.Rows.Add(row);

            }

        }



        private void FTEmployeeButton_Click(object sender, EventArgs e)

        {

            List<Employee> employees = ReadJsonData();



            //LINQ Query for FT Employees

            var FTEmp = from emp in employees

                        where emp.GetTaxForm == "W2"

                        select emp;



            //Display into DataGridView

            FillEmployeeDataGrid(FTEmp.ToList());

        }



        private void ContractEmployeeButton_Click(object sender, EventArgs e)

        {

            //Read from JSON file

            List<Employee> employees = ReadJsonData();



            //LINQ Query for Contract Employees

            var contractEmp = from emp in employees

                              where emp.GetTaxForm == "1099"

                              select emp;

            //Display into DataGridView

            FillEmployeeDataGrid(contractEmp.ToList());

        }



        //Method to determine developer type

        string typeName;

        public string SetDevType(int id)

        {

            if (id == 1)

            {

                typeName = "Object-Oriented";

            }

            else if (id == 2)

            {

                typeName = "Scripts";

            }

            else { typeName = "Unknown"; }

            return typeName;

        }



        public double AnnualPay(double amount) => 12 * amount;