C# Better implementation of JSON file

Question

This is homework so if you can help thank you, if not I understand.

Everything work as intended. But I feel like I could implement things better or have cleaner code. I would like the button to just call a method that reads the JSON file, runs the inquiry and puts out the correct display without duplicating the code like 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);
}

}

If this is code that works already, you should post on [Code Review.](https://codereview.stackexchange.com/) — maccettura, Nov 19 '18 at 20:28

Hanjun Chen · Answer 1 · 2018-11-19T21:19:17.183

I would say this line:

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

should be

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

for this reason.

local variables, private fields and parameters should be lowercase camel case, according to StyleCop:

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

The other thing is don't repeat yourself (DRY). These few lines can be refactored to be a separate method:

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;

In SetDevType you can totally use switch/case, which performs better.

string typeName; // why do we need this?
private string SetDevType(int id)
{
    string typeName = string.Empty;
    switch(id){
        case 1: typeName = "Something"; break;
        default: typeName = "Unknown";
    }
    return typeName;
}

some members of the class don't need to be public, such as

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

can be

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

and this method is somehow also in the Employee class, which is a Model class/POCO. POCO classes usually don't contain non-properties (although sometimes ToString and an empty constructor are included).

Thank you! I had the switch statement at one time and for somereason changed it. Looks cleaner as you said. The AnnualPay method was a result of cut and paste, thank for catching that. Yeah making a method of the repeated methods in the button_click is what I'm trying to fix. — Christopher Wood, Nov 20 '18 at 00:35

score 0 · Answer 2 · answered Nov 19 '18 at 21:37

As I can see your Click event handlers are very similar.

I see only one change:

In your FTEmployeeButton_Click event handler you filter data like that:

var contractEmp = from emp in employees
                  where emp.GetTaxForm == "1099"
                  select emp;

And in ContractEmployeeButton_Click event handler you filter data like that:

var FTEmp = from emp in employees
            where emp.GetTaxForm == "W2"
            select emp;

Other code looks very similar. So we can define your code to one separate method...

But we need understand how filter your data.

For this purpose I suggest to use Tag property of Button class.

In this property you can save any object. In your case you can save GetTaxForm value.

So you can use only one event handler for all your buttons.

Please look next realization:

private void OnButton_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);

    object filter = ((Control)sender).Tag;
    List<Employee> FTEmp;

    if (filter != null) {
        //LINQ Query for FT Employees
        FTEmp = from emp in employees
                    where emp.GetTaxForm == filter.ToString()
                   select emp;
    }
    else 
    {
       FTEmp = employees.ToList();
    }

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

I think that's my struggle. I want to make a method to read the file, and a method to provide the output. Then just run the method with the button click and the specific button has the LINQ query. I've tried what you showed here and didn't really get it. — Christopher Wood, Nov 20 '18 at 02:30

C# Better implementation of JSON file

2 Answers2