2
\$\begingroup\$

In this project (C# Win forms) I'm supposed to calculate wages of employees.

These are the steps:

  1. Calculate earnings from Attendance data. Here all regular earnings like BasicSalary, OverTimeAmount etc will be calculated.

  2. Then these earning details will be shown in a grid where user can enter values for blank columns (non fixed earning figures like SpecialAllowance which is not attendance based) (See class Earning)

  3. Then user can save these earning details on a table called SalaryTrans. (Each record on the grid has a Save button.

  4. Calculate deduction. For all the records (employees) which is in SalaryTrans, deductions will be calculated. (see class Deduction)

  5. Then these deduction details will be shown in a grid where user can enter non fixed / random deductions like OtherDeductions etc.

  6. Then user can save these deduction details on the same table SalaryTrans. (Actually in this case it updates deduction related columns of current records existing in the table.

  7. Then user can balance wages [BalanceWage = Earnings-Deduction].

  8. After balancing , Net wage should be calculated deducting CarriedForwardAmount from BalanceWage. This carried forward amount is the less than 100 fraction of BalanceWage. (ex: if BalanceWage is 7,526.50 then CF amount is 26.50 and NetWage should be 7500.00)

Following diagram shows a portion of class relationships with regard to the above scenario.

enter image description here

Following code shows the solution I've implemented for the above case. This is MVP pattern (for this demonstration I've removed interface usage and only the concrete classes are shown).

WageInfo has a one to many relationship with Earning,Deduction and WageBalance. Hence WageInfo maintains three list List<Earning>, List<Deduction> and List<WageBalance> as this is performed in 3 steps by the user.

Calculate Earnings Code Explanation

When requested, DataService returns a DataTable with all earings. Then WageManager passes it to WageInfo to fill the EarningList with data (Earning Objects) and then this list is returned back to WageManager where it is returned back to presenter to show the details on the grid.

Note: Earnings, Deductions and WageBalances are shown on separate grids.

Could you please review this code and let me know whether this is an acceptable solution?

public class Earning 
{
    public int EmployeeID { get; set; }
    public string Name { get; set; }
    public decimal BasicSalaryAmount { get; set; }
    public decimal BudjetoryAllowance { get; set; }
    public int WorkDays { get; set; }
    public int DayOffs { get; set; }
    public int LeaveDays { get; set; }
    public int AbsentDays { get; set; }
    public int ExtraShifts { get; set; }
    public int NoPayDays { get; set; }
    public decimal LessNoPayAmount { get; set; }
    public decimal AmountForEPF { get; set; }
    public decimal OverTimeAmount { get; set; }
    public decimal IncentiveAllowance { get; set; }
    public decimal SpecialAllowance { get; set; }
    public decimal OtherAllowance { get; set; }
    public decimal ExtraShiftAmount { get; set; }
    public decimal BroughtForwardAmount { get; set; }
    public string Remarks { get; set; }
}


             class WageInfo
                {
                    public DateTime WagePeriodStartDate { get; private set; }
                    public DateTime WagePeriodEndDate { get; private set; } 
                    public string Reference { get; private set; }

                    public Deduction Deduction;
                    public List<Deduction> DeductionList;

                    private Earning Earning;
                    private List<Earning> EarningList;

                    public WageBalance WageBalance;
                    public List<WageBalance> WageBalanceList;

                    public WageInfo()
                    {
                        Deduction = new Deduction();
                        DeductionList = new List<Payroll.Deduction>();

                        Earning = new Earning();
                        EarningList = new List<Payroll.Earning>();

                        WageBalance = new WageBalance();
                        WageBalanceList = new List<WageBalance>();

                    }

                    public Earning AddEarning(int employeeID, int workDays, int dayOffs, int leaveDays, int extraShifts, decimal extraShiftsAmount, decimal basicSalary, decimal budjetoryAllowance, 
                        int noPayDays, decimal lessNoPayAmount, decimal amountForEpf, decimal overTimeAmount, decimal broughtForwardAmount, decimal incentiveAllowance, decimal otherAllowance, decimal specialAllowance)
                    {
                        Earning.EmployeeID = employeeID;
                        Earning.WorkDays = workDays;
                        Earning.DayOffs = dayOffs;
                        Earning.LeaveDays = leaveDays;
                        Earning.ExtraShifts = extraShifts;
                        Earning.BasicSalaryAmount = basicSalary;
                        Earning.BudjetoryAllowance = budjetoryAllowance;
                        Earning.NoPayDays = noPayDays;
                        Earning.LessNoPayAmount = lessNoPayAmount;
                        Earning.AmountForEPF = amountForEpf;
                        Earning.OverTimeAmount = overTimeAmount;
                        Earning.ExtraShiftAmount = extraShiftsAmount;
                        Earning.IncentiveAllowance = incentiveAllowance;
                        Earning.OtherAllowance = otherAllowance;
                        Earning.SpecialAllowance=specialAllowance;
                        Earning.BroughtForwardAmount = broughtForwardAmount;

                        return Earning;
                    }


                    public List<Earning> CreateEarningList(DataTable dt)
                    {
                        EarningList= dt.Rows.OfType<DataRow>().Select(InsertEarningsToList).ToList();
                        return EarningList;

                    }

                    private Earning InsertEarningsToList(DataRow row)
                    {
                        WageInfo wi = new WageInfo();

                        return wi.AddEarning(

                        row["Emp_ID"] == DBNull.Value ? 0 : Convert.ToInt32(row[0]),
                        row[2] == DBNull.Value ? 0 : Convert.ToInt32(row[2]),
                        row[3] == DBNull.Value ? 0 : Convert.ToInt32(row[3]),
                        row[4] == DBNull.Value ? 0 : Convert.ToInt32(row[4]),
                        row[5] == DBNull.Value ? 0 : Convert.ToInt32(row[5]),
                        row[7] == DBNull.Value ? 0 : Convert.ToDecimal(row[7]),
                        row[8] == DBNull.Value ? 0 : Convert.ToDecimal(row[8]),
                        row[9] == DBNull.Value ? 0 : Convert.ToDecimal(row[9]),
                        row[10] == DBNull.Value ? 0 : Convert.ToInt32(row[10]),
                        row[11] == DBNull.Value ? 0 : Convert.ToDecimal(row[11]),
                        row[12] == DBNull.Value ? 0 : Convert.ToDecimal(row[12]),
                        row[13] == DBNull.Value ? 0 : Convert.ToDecimal(row[13]),
                        row[14] == DBNull.Value ? 0 : Convert.ToDecimal(row[14]),
                        row[15] == DBNull.Value ? 0 : Convert.ToDecimal(row[15]),
                        row[16] == DBNull.Value ? 0 : Convert.ToDecimal(row[16]),
                        row[15] == DBNull.Value ? 0 : Convert.ToDecimal(row[17])
                        );
                    }

                    public Earning UpdateEarnings(int row)
                    {
                        return EarningList[row];

                    }


        class WageManager
            {
                WageInfo _WageInfo;
                DataService _DataService;


                public WageManager(WageInfo wageInfo, DataService dataService )
                {
                    _WageInfo = wageInfo;
                    _DataService = dataService;
                }

                # region Earning



                public List<Earning> PrepareEarnings(DateTime wagePeriodStartDate, DateTime wagePeriodEndDate)
                {
                    var info = _DataService.GetEarnings(wagePeriodStartDate, wagePeriodEndDate);
                    return _WageInfo.CreateEarningList(info);
                }

                public void UpdateEarnings(int row)
                {
                    _DataService.InsertEarnings( _WageInfo.UpdateEarnings(row));

                }
            }


     class WagesPresenter : BasePresenter
        {
            WageInfo _WageInfo;
            frmWages _WageView;
            WageManager _WageManager;

            BindingSource bS = new BindingSource();

            public WagesPresenter(WageManager wageManger, WageInfo wageInfo, frmWages wageView  )
            {
                this._WageInfo = wageInfo;
                this._WageView = wageView;
                _WageManager = wageManger;  
            }

    private void _WageView_OnProcessingEarnings(object sender, EventArgs e)
            {
                ShowEarnings();
            }


            private void ShowEarnings()
            {
                bS.DataSource = _WageManager.PrepareEarnings(_WageInfo.WagePeriodStartDate, _WageInfo.WagePeriodEndDate);

                _WageView.EarningDetails = bS;
            }
    }


class DataService
{
        public DataTable GetEarnings(DateTime fromDate, DateTime toDate)
        {
            using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
            {
                using (SqlCommand sqlCommand = new SqlCommand("dbo.sp_Earnings", sqlConnection))
                {
                    sqlCommand.CommandType = CommandType.StoredProcedure;
                    sqlCommand.Parameters.Add("@fromDate", SqlDbType.DateTime).Value = fromDate;
                    sqlCommand.Parameters.Add("@toDate", SqlDbType.DateTime).Value = toDate;
                    using (SqlDataAdapter sqlAdapter = new SqlDataAdapter(sqlCommand))
                    {
                        using (DataSet dataSet = new DataSet())
                        using (DataTable dataTable = new DataTable())
                        {
                            sqlConnection.Open();
                            dataSet.Tables.Add(dataTable);
                            sqlAdapter.Fill(dataTable);
                            return dataTable;
                        }
                    }
                }
            }
        }
}
Improve this question
\$\endgroup\$
0

3 Answers 3

Reset to default
3
\$\begingroup\$

It seems there is enough missing here it is hard to determine intent of some of your code so I just made assumptions and changed as I thought some good practices would dictate the code to be.

I think the usage of public members is really bad. It allows for anyone to modify them in any way they please. I always take the stance of build your classes as if someone is going to try to abuse them, either by passing in nulls, or overwriting public members. This forces you to think how to encapsulate as much data and only expose the bare minimums.

From the code provided I couldn't see a reason for the public members of: DeductionList, EarningList and WageBalanceList. If you want to share those with people you could make them properties with a public get and a private set, but I wouldn't even expose the list unless you expose a readonly version, because if you return the list from the get someone can just add whatever earnings they please to the list and it will mutate the state inside the WageInfo class.

The AddEarnings method has a horrid signature. Are there various earning scenarios they don't require as many parameters? Perhaps you could create a few that show more purposeful intent and require less parameters. My comment later in your code about the parsing of the row and calling AddEarning makes that extremely difficult to maintain. I also would suggest when you see repetition (the parsing) encapsulate in to a method.

I also don't understand why you have public members of single instance of each of the Deduction, Earning and WageBalance classes. Looking at the code if the WageInfo AddEarning is called more than once you are changing the single instance of the member, is that correct? Furthermore, should AddEarning be just mutating the internal state of the WageInfo?

Code cleaned up:

public class Earning 
   {
    public int EmployeeID { get; set; }
    public string Name { get; set; }
    public decimal BasicSalaryAmount { get; set; }
    public decimal BudjetoryAllowance { get; set; }
    public int WorkDays { get; set; }
    public int DayOffs { get; set; }
    public int LeaveDays { get; set; }
    public int AbsentDays { get; set; }
    public int ExtraShifts { get; set; }
    public int NoPayDays { get; set; }
    public decimal LessNoPayAmount { get; set; }
    public decimal AmountForEPF { get; set; }
    public decimal OverTimeAmount { get; set; }
    public decimal IncentiveAllowance { get; set; }
    public decimal SpecialAllowance { get; set; }
    public decimal OtherAllowance { get; set; }
    public decimal ExtraShiftAmount { get; set; }
    public decimal BroughtForwardAmount { get; set; }
    public string Remarks { get; set; }
}


class WageInfo
{
public DateTime WagePeriodStartDate { get; private set; }
public DateTime WagePeriodEndDate { get; private set; } 
public string Reference { get; private set; }

//delete the single instance of the classes and made these private
private List<Deduction> DeductionList;

private List<Earning> EarningList;

private List<WageBalance> WageBalanceList;

public WageInfo()
{
    DeductionList = new List<Payroll.Deduction>();

    EarningList = new List<Payroll.Earning>();

    WageBalanceList = new List<WageBalance>();
}

public Earning AddEarning(int employeeID, int workDays, int dayOffs, int leaveDays, int extraShifts, decimal extraShiftsAmount, decimal basicSalary, decimal budjetoryAllowance, 
    int noPayDays, decimal lessNoPayAmount, decimal amountForEpf, decimal overTimeAmount, decimal broughtForwardAmount, decimal incentiveAllowance, decimal otherAllowance, decimal specialAllowance)
{
    Earning = new Earning();
    Earning.EmployeeID = employeeID;
    Earning.WorkDays = workDays;
    Earning.DayOffs = dayOffs;
    Earning.LeaveDays = leaveDays;
    Earning.ExtraShifts = extraShifts;
    Earning.BasicSalaryAmount = basicSalary;
    Earning.BudjetoryAllowance = budjetoryAllowance;
    Earning.NoPayDays = noPayDays;
    Earning.LessNoPayAmount = lessNoPayAmount;
    Earning.AmountForEPF = amountForEpf;
    Earning.OverTimeAmount = overTimeAmount;
    Earning.ExtraShiftAmount = extraShiftsAmount;
    Earning.IncentiveAllowance = incentiveAllowance;
    Earning.OtherAllowance = otherAllowance;
    Earning.SpecialAllowance=specialAllowance;
    Earning.BroughtForwardAmount = broughtForwardAmount;

    return Earning;
}


public List<Earning> CreateEarningList(DataTable dt)
{

    EarningList= dt.Rows.OfType<DataRow>().Select(InsertEarningsToList).ToList();
    //consider making this list read only like: EarningList.AsReadOnly()
    return EarningList;

}

private Earning InsertEarningsToList(DataRow row)
{
    WageInfo wi = new WageInfo();
    //this is a wall of junk, it extremely hard to know which parameter is which in the method
    return wi.AddEarning(

    row["Emp_ID"] == DBNull.Value ? 0 : Convert.ToInt32(row[0]),
    GetIntOrDefault(row,2),
    GetIntOrDefault(row,3),
    GetIntOrDefault(row,4),
    GetIntOrDefault(row,5),
    GetDecimalOrDefault(row,6),
    GetDecimalOrDefault(row,8),
    GetDecimalOrDefault(row,9),
    GetIntOrDefault(row,10),
    GetDecimalOrDefault(row,11),
    GetDecimalOrDefault(row,12),
    GetDecimalOrDefault(row,13),
    GetDecimalOrDefault(row,14),
    GetDecimalOrDefault(row,15),
    GetDecimalOrDefault(row,16),
    GetDecimalOrDefault(row,17)
    );
}

private int GetIntOrDefault(DataRow row, int index) 
{
    return row[index] == DBNull.Value ? 0 : Convert.ToInt32(row[index]);
}

private int GetDecimalOrDefault(DataRow row, int index) 
{
    return row[index] == DBNull.Value ? 0 : Convert.ToDecimal(row[index]);
}

public Earning UpdateEarnings(int row)
{
    return EarningList[row];

}
}


class WageManager
{

WageInfo _WageInfo;
DataService _DataService;


public WageManager(WageInfo wageInfo, DataService dataService )
{
    _WageInfo = wageInfo;
    _DataService = dataService;
}

public List<Earning> PrepareEarnings(DateTime wagePeriodStartDate, DateTime wagePeriodEndDate)
{
    var info = _DataService.GetEarnings(wagePeriodStartDate, wagePeriodEndDate);
    return _WageInfo.CreateEarningList(info);
}

public void UpdateEarnings(int row)
{
    _DataService.InsertEarnings( _WageInfo.UpdateEarnings(row));

}
}




class WagesPresenter : BasePresenter
{
WageInfo _WageInfo;
frmWages _WageView;
WageManager _WageManager;

BindingSource bS = new BindingSource();

public WagesPresenter(WageManager wageManger, WageInfo wageInfo, frmWages wageView  )
{
    this._WageInfo = wageInfo;
    this._WageView = wageView;
    _WageManager = wageManger;  
}

private void _WageView_OnProcessingEarnings(object sender, EventArgs e)
{
    ShowEarnings();
}


private void ShowEarnings()
{
    bS.DataSource = _WageManager.PrepareEarnings(_WageInfo.WagePeriodStartDate, _WageInfo.WagePeriodEndDate);

    _WageView.EarningDetails = bS;
}
}


class DataService
{
public DataTable GetEarnings(DateTime fromDate, DateTime toDate)
{
    using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
    {
        using (SqlCommand sqlCommand = new SqlCommand("dbo.sp_Earnings", sqlConnection))
        {
            sqlCommand.CommandType = CommandType.StoredProcedure;
            sqlCommand.Parameters.Add("@fromDate", SqlDbType.DateTime).Value = fromDate;
            sqlCommand.Parameters.Add("@toDate", SqlDbType.DateTime).Value = toDate;
            using (SqlDataAdapter sqlAdapter = new SqlDataAdapter(sqlCommand))
            {
                using (DataSet dataSet = new DataSet())
                using (DataTable dataTable = new DataTable())
                {
                    sqlConnection.Open();
                    dataSet.Tables.Add(dataTable);
                    sqlAdapter.Fill(dataTable);
                    return dataTable;
                }
            }
        }
    }
}
}
Improve this answer
\$\endgroup\$
1
5
\$\begingroup\$

This method smells:

public Earning AddEarning(int employeeID, int workDays, int dayOffs, int leaveDays, int extraShifts, decimal extraShiftsAmount, decimal basicSalary, decimal budjetoryAllowance,                              int noPayDays, decimal lessNoPayAmount, decimal amountForEpf, decimal overTimeAmount, decimal broughtForwardAmount, decimal incentiveAllowance, decimal otherAllowance, decimal specialAllowance)

There are way too many parameters, and it's not adding anything anywhere: it's just a factory method that's not really a factory method because it's affecting the internal state of the WageInfo class, which just feels wrong.

I'd be expecting something like this of a factory method:

public Earning CreateEarning(int employeeId, int workDays, ...)
{
    var result = new Earning { EmployeeId = employeeId, WorkDays = workDays, ... };
    return result;
}

But what's a factory method that creates Earning instances doing in a WageInfo class?

An AddEarning method would take a single Earning earning parameter, add it to the internal earnings list, and return void. Looking at some more of the code it seems that's what InsertEarningsToList is doing - the return statement is pretty much obfuscated, I almost cast a close vote on your question for being non-working code. I don't see the need for creating a new WageInfo within WageInfo just to return a Earning instance.

What you have here is spaghetti code, you need to sort it out.

Improve this answer
\$\endgroup\$
1
2
\$\begingroup\$

Naming Convention

Use camelCase for private fields.

//WageInfo _WageInfo;
//DataService _DataService;
WageInfo wageInfo;
DataService dataService;

Avoid prefixing with _underscore. Use thiskeyword to tell the compile you mean the private memeber

public WageManager(WageInfo wageInfo, DataService dataService)
{
    //_WageInfo = wageInfo;
    //_DataService = dataService;
    this.wageInfo = wageInfo;
    this.dataService = dataService;
}

Refactoring

Avoid repeating yourself, extract the frequent used code segment.

public static class DataRowExtensions
{
    public static int GetInt(this DataRow row, string columnName, int defaultValue = 0)
    {
        if (row.IsNull(columnName)) return defaultValue;

        return Convect.ToInt32(row[columnName]);
    }
    public static decimal GetDecimal(this DataRow row, string columnName, decimal defaultValue = 0)
    {
        if (row.IsNull(columnName)) return defaultValue;

        return Convect.ToDecimal(row[columnName]);
    }
}

    return new Earning
    {
        //EmployeeID = row["Emp_ID"] == DBNull.Value ? 0 : Convert.ToInt32(row["Emp_ID"]),
        //WorkDays = row["work_days"] == DBNull.Value ? 0 : Convert.ToInt32(row["work_days"]),
        //DayOffs = row["day_offs"] == DBNull.Value ? 0 : Convert.ToInt32(row["day_offs"]),
        //LeaveDays = row["leave_days"] == DBNull.Value ? 0 : Convert.ToInt32(row["leave_days"]),
        //ExtraShifts = row["extra_shifts"] == DBNull.Value ? 0 : Convert.ToInt32(row["extra_shifts"]),
        //ExtraShiftAmount = row["Extra_Shift_Amount"] == DBNull.Value ? 0 : Convert.ToDecimal(row["Extra_Shift_Amount"]),
        //BasicSalaryAmount = row["Basic_Sal"] == DBNull.Value ? 0 : Convert.ToDecimal(row["Basic_Sal"]),
        //BudjetoryAllowance = row["budj_allowance"] == DBNull.Value ? 0 : Convert.ToDecimal(row["budj_allowance"]),
        //NoPayDays = row["no_pay_days"] == DBNull.Value ? 0 : Convert.ToInt32(row["no_pay_days"]),
        //LessNoPayAmount = row["less_no_pay_amt"] == DBNull.Value ? 0 : Convert.ToInt32(row["less_no_pay_amt"]),
        //AmountForEPF = row["amt_for_epf"] == DBNull.Value ? 0 : Convert.ToDecimal(row["amt_for_epf"]),
        //OverTimeAmount = row["over_time_amt"] == DBNull.Value ? 0 : Convert.ToDecimal(row["over_time_amt"]),
        //BroughtForwardAmount = row["cf_amount"] == DBNull.Value ? 0 : Convert.ToDecimal(row["cf_amount"]),
        //IncentiveAllowance = row["incentive_allowance"] == DBNull.Value ? 0 : Convert.ToDecimal(row["incentive_allowance"]),
        //OtherAllowance = row["other_allowance"] == DBNull.Value ? 0 : Convert.ToDecimal(row["other_allowance"]),
        //SpecialAllowance = row["special_allowance"] == DBNull.Value ? 0 : Convert.ToDecimal(row["special_allowance"])

        EmployeeID = row.GetInt("Emp_ID"),
        //...
    };
Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ +1 but this is a much longer prefix than _ - it's essentially down to personal preference, what matters is consistency... and your code assigns the parameters to their respective values, did you mean to prefix with this.? (oh the irony! ;) \$\endgroup\$
    – Mathieu Guindon
    May 27, 2014 at 18:26
  • \$\begingroup\$ The This. prefix is for explicitly tell the compiler to refer to the member variable rather than the parameter. This is only required to use in the constructor to avoid confusion. Anywhere else you can refer it directly with wageInfo. \$\endgroup\$
    – Xiaoy312
    May 27, 2014 at 18:49
  • \$\begingroup\$ ...Unless there's a method with a wageInfo parameter. Let's just agree to disagree here, I prefer having an underscore prefix on my private fields, and whether I do that or use a this. prefix doesn't matter, as long as the code is consistent about it ;) that said, the names with the underscore prefix should be like _wageInfo, not _WageInfo. \$\endgroup\$
    – Mathieu Guindon
    May 27, 2014 at 18:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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