4
\$\begingroup\$

I'm using the following query for multiple transactions. This is the scenario.

Generate Salary (this is not in the scope of the query) I have employee attendance in Attendance table. Then the salary is calculate from the attendance by using a stored procedure and resulting figures will be displayed in an interface allowing users to monitor / modify (entering allowances etc).

THE QUERY

  • When the user press save, the salary figures should be inserted to SalaryTrans table.

  • If salaries are regenerated and re saved the user then the existing
    figures should be updated. (After modifying attendance or salary figures such as allowances etc)

  • When regenerated, in case if an employee who was in the attendance
    earlier but now has been deleted later from attendance, then the salary
    figures already in SalaryTrans table should also be deleted. (To
    ensure that only the employees in attendance table are the ones in
    SalaryTrans table)

The following query was used for the above purpose and I want to know whether this approach is acceptable or not, and also any feedback regarding the logic and its implementation. So could you review this code please?

NOTE : The SalaryTrans table has a varChar field called reference to hold the month and year of salary (ex value. Jul2014)

The Attendance table has DateTime fields called in_time and out_time to hold on and off of attendance. This in_time field should be used when filtering data pertaining to a month. 

public bool InsertEarnings(List<Earning> earningsList, string reference, DateTime fromDate, DateTime toDate)
        {            
            using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
            {
                string insertStatement = "IF NOT EXISTS (SELECT * FROM SalaryTrans WHERE employee_id=@employee_id and reference = @reference )" +
                                         "INSERT INTO salaryTrans " +
                                         "(employee_id, work_days, day_offs, leave_days, absent_days, extra_shifts, basic_salary, budjetory_allowance, no_pay_days, less_no_pay_amount, amount_for_epf, over_time_amount, extra_shifts_amount, incentive_allowance, special_allowance, other_allowance, epf, brought_forward_amount, reference ) " +
                                         "VALUES " +
                                         "(@employee_id, @work_days, @day_offs, @leave_days, @absent_days, @extra_shifts, @basic_salary, @budjetory_allowance, @no_pay_days, @less_no_pay_amount, @amount_for_epf, @over_time_amount, @extra_shifts_amount, @incentive_allowance, @special_allowance, @other_allowance, @epf, @brought_forward_amount, @reference ) " +
                                         "ELSE " +
                                         "UPDATE SalaryTrans " +
                                         "SET work_days=@work_days, day_offs=@day_offs, leave_days=@leave_days, absent_days=@absent_days, extra_shifts=@extra_shifts, basic_salary=@basic_salary, budjetory_allowance=@budjetory_allowance, no_pay_days=@no_pay_days, less_no_pay_amount=@less_no_pay_amount, amount_for_epf=@amount_for_epf," +
                                         "over_time_amount = @over_time_amount, extra_shifts_amount=@extra_shifts_amount, incentive_allowance=@incentive_allowance, special_allowance=@special_allowance, other_allowance=@other_allowance, epf=@epf, brought_forward_amount=@brought_forward_amount " +
                                         "WHERE employee_id=@employee_id AND reference = @reference " +
                                         "DELETE FROM SalaryTrans " +
                                         "WHERE reference = @reference AND SalaryTrans.employee_id NOT IN " +
                                         "(SELECT EID " +
                                         "FROM Attendance " +
                                         "WHERE (in_time BETWEEN CONVERT(DATETIME, @fromDate, 102) AND CONVERT(DATETIME, @toDate, 102))) ";

                using (SqlCommand sqlCommand = new SqlCommand(insertStatement, sqlConnection))
                {
                    SqlParameter paramEmployeeID = new SqlParameter("@employee_id", SqlDbType.Char);
                    SqlParameter paramWorDays = new SqlParameter("@work_days", SqlDbType.Int);
                    SqlParameter paramDayOffs = new SqlParameter("@day_offs", SqlDbType.Int);
                    SqlParameter paramLeaveDays = new SqlParameter("@leave_days", SqlDbType.Int);
                    SqlParameter paramAbsentDays = new SqlParameter("@absent_days", SqlDbType.Int);
                    SqlParameter paramExtraShifts = new SqlParameter("@extra_shifts", SqlDbType.Int);
                    SqlParameter paramBasicSalary = new SqlParameter("@basic_salary", SqlDbType.Decimal);
                    SqlParameter paramBudjAllowance = new SqlParameter("@budjetory_allowance", SqlDbType.Decimal);
                    SqlParameter paramNoPayDays = new SqlParameter("@no_pay_days", SqlDbType.Int);
                    SqlParameter paramLessNoPay = new SqlParameter("@less_no_pay_amount", SqlDbType.Decimal);
                    SqlParameter paramAmountForEPF = new SqlParameter("@amount_for_epf", SqlDbType.Decimal);
                    SqlParameter paramOverTime = new SqlParameter("@over_time_amount", SqlDbType.Decimal);
                    SqlParameter paramExtraShiftsAmount = new SqlParameter("@extra_shifts_amount", SqlDbType.Decimal);
                    SqlParameter paramIncentiveAllowance = new SqlParameter("@incentive_allowance", SqlDbType.Decimal);
                    SqlParameter paramSpecialAllowance = new SqlParameter("@special_allowance", SqlDbType.Decimal);
                    SqlParameter paramOtherAllowance = new SqlParameter("@other_allowance", SqlDbType.Decimal);
                    SqlParameter paramBFAmount = new SqlParameter("@brought_forward_amount", SqlDbType.Decimal);
                    SqlParameter paramEpf = new SqlParameter("@epf", SqlDbType.Decimal);
                    SqlParameter paramReference = new SqlParameter("@reference", SqlDbType.VarChar);
                    SqlParameter paramFromDate = new SqlParameter("@fromDate", SqlDbType.DateTime);
                    SqlParameter paramToDate = new SqlParameter("@toDate", SqlDbType.DateTime);

                    sqlCommand.Parameters.Add(paramEmployeeID);
                    sqlCommand.Parameters.Add(paramWorDays);
                    sqlCommand.Parameters.Add(paramDayOffs);
                    sqlCommand.Parameters.Add(paramLeaveDays);
                    sqlCommand.Parameters.Add(paramAbsentDays);
                    sqlCommand.Parameters.Add(paramExtraShifts);
                    sqlCommand.Parameters.Add(paramBasicSalary);
                    sqlCommand.Parameters.Add(paramBudjAllowance);
                    sqlCommand.Parameters.Add(paramNoPayDays);
                    sqlCommand.Parameters.Add(paramLessNoPay);
                    sqlCommand.Parameters.Add(paramAmountForEPF);
                    sqlCommand.Parameters.Add(paramOverTime);
                    sqlCommand.Parameters.Add(paramExtraShiftsAmount);
                    sqlCommand.Parameters.Add(paramIncentiveAllowance);
                    sqlCommand.Parameters.Add(paramSpecialAllowance);
                    sqlCommand.Parameters.Add(paramOtherAllowance);
                    sqlCommand.Parameters.Add(paramBFAmount);
                    sqlCommand.Parameters.Add(paramEpf);
                    sqlCommand.Parameters.Add(paramReference);
                    sqlCommand.Parameters.Add(paramFromDate);
                    sqlCommand.Parameters.Add(paramToDate);

                        sqlConnection.Open();
                        SqlTransaction sqlTrans = sqlConnection.BeginTransaction("Insert");
                        sqlCommand.Transaction = sqlTrans;
                        try
                        {
                            foreach (Earning earning in earningsList)
                            {
                                paramEmployeeID.Value = earning.EmployeeID;
                                paramWorDays.Value = earning.WorkDays;
                                paramDayOffs.Value = earning.DayOffs;
                                paramLeaveDays.Value = earning.LeaveDays;
                                paramAbsentDays.Value = earning.AbsentDays;
                                paramExtraShifts.Value = earning.ExtraShifts;
                                paramBasicSalary.Value = earning.BasicSalaryAmount;
                                paramBudjAllowance.Value = earning.BudjetoryAllowance;
                                paramNoPayDays.Value = earning.NoPayDays;
                                paramLessNoPay.Value = earning.LessNoPayAmount;
                                paramAmountForEPF.Value = earning.AmountForEPF;
                                paramOverTime.Value = earning.OverTimeAmount;
                                paramExtraShiftsAmount.Value = earning.ExtraShiftAmount;
                                paramIncentiveAllowance.Value = earning.IncentiveAllowance;
                                paramSpecialAllowance.Value = earning.SpecialAllowance;
                                paramOtherAllowance.Value = earning.OtherAllowance;
                                paramBFAmount.Value = earning.BroughtForwardAmount;
                                paramEpf.Value = earning.AmountForEPF / 100 * 8;
                                paramReference.Value = reference;
                                paramToDate.Value = toDate;
                                paramFromDate.Value = fromDate;

                                sqlCommand.ExecuteNonQuery();
                            }
                            sqlTrans.Commit();
                            return true;
                    }
                    catch (Exception e) { MessageBox.Show(("Error: " + e.Message)); }
                    sqlTrans.Rollback();
                    if (sqlConnection.State == System.Data.ConnectionState.Open) sqlConnection.Close();
                    return false;
                }
            }
        }
Improve this question
\$\endgroup\$
4
  • \$\begingroup\$ This code doesn't seem to be working as it is. \$\endgroup\$
    – Phrancis
    Jul 29, 2014 at 14:16
  • \$\begingroup\$ @ Phrancis, @200_success♦, Well, I had posted only the query part of the code as the code is very lengthy. But is a working code. Anyway I edited the question and now full code is available. Hope you would reopen the post. \$\endgroup\$
    – CAD
    Jul 29, 2014 at 16:45
  • 1
    \$\begingroup\$ That's actually a much more interesting question to review, with the real code. \$\endgroup\$
    – 200_success
    Jul 29, 2014 at 16:47
  • \$\begingroup\$ are employee salary generation is related to each other? if not why transaction has been opened before loop, your transnational code should be small, \$\endgroup\$
    – Paritosh
    Jul 30, 2014 at 5:23

1 Answer 1

Reset to default
7
\$\begingroup\$

As this method is quite large, let's start digging through the code.

  • insertStatement should be a const on class level

  • The creation of the SqlParameters can be extracted to a method

    private SqlParameter[] GetInsertEarningsParameters()
{
    //I use a List<T> because it is easier to read.
    List<SqlParameter> sqlParameters = new List<SqlParameter>();

    sqlParameters.Add(new SqlParameter("@employee_id", SqlDbType.Char));
    sqlParameters.Add(new SqlParameter("@work_days", SqlDbType.Int));
    sqlParameters.Add(new SqlParameter("@day_offs", SqlDbType.Int));
    sqlParameters.Add(new SqlParameter("@leave_days", SqlDbType.Int));
    sqlParameters.Add(new SqlParameter("@absent_days", SqlDbType.Int));
    sqlParameters.Add(new SqlParameter("@extra_shifts", SqlDbType.Int));
    sqlParameters.Add(new SqlParameter("@basic_salary", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@budjetory_allowance", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@no_pay_days", SqlDbType.Int));
    sqlParameters.Add(new SqlParameter("@less_no_pay_amount", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@amount_for_epf", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@over_time_amount", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@extra_shifts_amount", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@incentive_allowance", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@special_allowance", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@other_allowance", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@brought_forward_amount", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@epf", SqlDbType.Decimal));
    sqlParameters.Add(new SqlParameter("@reference", SqlDbType.VarChar));
    sqlParameters.Add(new SqlParameter("@fromDate", SqlDbType.DateTime));
    sqlParameters.Add(new SqlParameter("@toDate", SqlDbType.DateTime));

    return sqlParameters.ToArray();
}

    and then be called

    sqlCommand.Parameters.AddRange(GetInsertEarningsParameters());

  • As the values for reference, toDate and fromDate won't change, we will assign the parameter values before the foreach loop only once

    sqlCommand.Parameters["@reference"].Value = reference;
sqlCommand.Parameters["@toDate"].Value = toDate;
sqlCommand.Parameters["@fromDate"].Value = fromDate;

  • The assignments of the values of the Earning's object should be extracted to a method and the magic number 100 * 8 should be a const

    private const int AMOUNT_FOR_EPF_DIVISOR = 100 * 8;
private void FillInsertEarningsParameterValues(Earning earning, SqlParameterCollection parameters)
{
    parameters["@employee_id"].Value = earning.EmployeeID;
    parameters["@work_days"].Value = earning.WorkDays;
    parameters["@day_offs"].Value = earning.DayOffs;
    parameters["@leave_days"].Value = earning.LeaveDays;
    parameters["@absent_days"].Value = earning.AbsentDays;
    parameters["@extra_shifts"].Value = earning.ExtraShifts;
    parameters["@basic_salary"].Value = earning.BasicSalaryAmount;
    parameters["@budjetory_allowance"].Value = earning.BudjetoryAllowance;
    parameters["@no_pay_days"].Value = earning.NoPayDays;
    parameters["@less_no_pay_amount"].Value = earning.LessNoPayAmount;
    parameters["@amount_for_epf"].Value = earning.AmountForEPF;
    parameters["@over_time_amount"].Value = earning.OverTimeAmount;
    parameters["@extra_shifts_amount"].Value = earning.ExtraShiftAmount;
    parameters["@incentive_allowance"].Value = earning.IncentiveAllowance;
    parameters["@special_allowance"].Value = earning.SpecialAllowance;
    parameters["@other_allowance"].Value = earning.OtherAllowance;
    parameters["@brought_forward_amount"].Value = earning.BroughtForwardAmount;
    parameters["@epf"].Value = earning.AmountForEPF / AMOUNT_FOR_EPF_DIVISOR;
}

    now the foreach will be

    foreach (Earning earning in earningsList)
{
    FillInsertEarningsParameterValues(earning,sqlCommand.Parameters);

    sqlCommand.ExecuteNonQuery();
}

  • The result of the refactoring of your large method would be

    public bool InsertEarnings(List<Earning> earningsList, string reference, DateTime fromDate, DateTime toDate)
{
    using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
    {

        using (SqlCommand sqlCommand = new SqlCommand(insertStatement, sqlConnection))
        {
            sqlCommand.Parameters.AddRange(GetInsertEarningsParameters());

            sqlCommand.Parameters["@reference"].Value = reference;
            sqlCommand.Parameters["@toDate"].Value = toDate;
            sqlCommand.Parameters["@fromDate"].Value = fromDate;

            sqlConnection.Open();
            SqlTransaction sqlTrans = sqlConnection.BeginTransaction("Insert");
            sqlCommand.Transaction = sqlTrans;
            try
            {
                foreach (Earning earning in earningsList)
                {
                    FillInsertEarningsParameterValues(earning, sqlCommand.Parameters);

                    sqlCommand.ExecuteNonQuery();
                }
                sqlTrans.Commit();
                return true;
            }
            catch (Exception e) { MessageBox.Show(("Error: " + e.Message)); }
            sqlTrans.Rollback();
            if (sqlConnection.State == System.Data.ConnectionState.Open) sqlConnection.Close();
            return false;
        }
    }
}
Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Nice review thanks. What about the SQL part? \$\endgroup\$
    – CAD
    Jul 31, 2014 at 9:32
  • \$\begingroup\$ @Chathuranga What do you mean with SQL part ? \$\endgroup\$
    – Heslacher
    Jul 31, 2014 at 9:35
  • \$\begingroup\$ I meant the query. If you would say its ok, it suffice \$\endgroup\$
    – CAD
    Jul 31, 2014 at 9:53
  • \$\begingroup\$ @Chathuranga I am not the expert with sql statements. You can extract the sql statement and create a new question with the sql tag \$\endgroup\$
    – Heslacher
    Jul 31, 2014 at 10:00

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.