Using JSON file for employee pay data

Question

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"
}
]

• user33306: If you could show the Employee class and a sample of the json file it would help people give you better answers.
• Christopher Wood: @tinstaafl the Employee class in the main code at the bottom

Answer 1

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.

Answer 2

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"));
}

Answer 3

You can simplify your code even farther yet. with a bit of refactoring in your class you can make properties out of the tax calculation and the net annual pay functions. a Dictionary for the formtypes, so that you can map different strings to the same value and a Dictionary to map the departmentid to the appropriate string, this eliminates the function for that:

class Employee
{
 readonly Dictionary<string,int> formTypes = new Dictionary<string, int>()
 {
 {"W2",0 },
 {"w2",0 },
 {"1099", 1 }
 };
 readonly Dictionary<int, string> devTypes = new Dictionary<int, string>()
 {
 {0,"Unknown" },
 {1,"Object-Oriented" },
 {2,"Scripts" }
 };
 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 DevType
 {
 get
 {
 return devTypes[DepartmentId];
 }
 }
 public double Tax
 {
 get
 {
 return CalculateTax(Pay);
 }
 }
 public double NetAnnualPay
 {
 get
 {
 return AnnualPay(Pay) - Tax;
 }
 }
 public string GetTaxForm { get; set; }
 private double CalculateTax(double basis)
 {
 double monthlyTax;
 if (formTypes[GetTaxForm] == 0)
 {
 monthlyTax = .07 * basis;
 }
 else
 {
 monthlyTax = 0;
 }
 return 12 * monthlyTax;
 }
 private double AnnualPay(double amount) => 12 * amount;
}

The advantage of this is that, instead of looping through each item in the list, you can load them automatically by setting the list as the datasource for the datagridview. To get the currency format, you can set the defaultcellstyle.format for those columns:

DataTable dt = new DataTable("Employee Data");
private void LoadAllButton_Click(object sender, System.EventArgs e)
{
 employees = JsonConvert.DeserializeObject<List<Employee>>(File.ReadAllText("JSON.json")); 
 dataGridView1.DataSource = employees;
 dataGridView1.Columns["Pay"].DefaultCellStyle.Format = "C";
 dataGridView1.Columns["Tax"].DefaultCellStyle.Format = "C";
 dataGridView1.Columns["NetAnnualPay"].DefaultCellStyle.Format = "C";
 var formTypes = employees.Select(x => x.GetTaxForm).Distinct().ToArray();
 comboBox1.Items.AddRange(formTypes);
}

To filter the data. I would suggest a combobox. the load method above shows how to dynamically load the combobox according to the different form types in the data. In the SelectedValueChanged event handler one can reload the datagridview with the filtered data:

private void comboBox1_SelectedValueChanged(object sender, System.EventArgs e)
{
 string formType = comboBox1.SelectedItem.ToString();
 var filteredList = (from emp in employees
 where emp.GetTaxForm == formType
 select emp).ToList();
 dataGridView1.DataSource = filteredList;
}

Answer 4

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;

• t3chb0t: Please take a look at the other reviews to see how to write them. Simply posting changed code isn't enough. You need to explain the changes you've made, or point things that can be improved in OP's code etc.