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;
                }
            }
        }
\$\endgroup\$
  • \$\begingroup\$ This code doesn't seem to be working as it is. \$\endgroup\$ – Phrancis Jul 29 '14 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 '14 at 16:45
  • 1
    \$\begingroup\$ That's actually a much more interesting question to review, with the real code. \$\endgroup\$ – 200_success Jul 29 '14 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 '14 at 5:23
6
\$\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;
            }
        }
    }
    
\$\endgroup\$
  • \$\begingroup\$ Nice review thanks. What about the SQL part? \$\endgroup\$ – CAD Jul 31 '14 at 9:32
  • \$\begingroup\$ @Chathuranga What do you mean with SQL part ? \$\endgroup\$ – Heslacher Jul 31 '14 at 9:35
  • \$\begingroup\$ I meant the query. If you would say its ok, it suffice \$\endgroup\$ – CAD Jul 31 '14 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 '14 at 10:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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