16
\$\begingroup\$

In the following code I'm trying to calculate Net Wage of employees and classes Earning, Deduction and WageBalance has a composition relationship with WageInfo. And there is a class called WageManager which is having a association with WageInfo, handling all operations like BalanceWage(). DataService class encapsulates all db logic. Please review my code and let me know of any flaws.

Is the way I've implemented the relationships correct? Would you have a method to calculate BalanceWage in a more elegant way?

enter image description here

    class WageManager
    {
        WageInfo _WageInfo;
        DataService _DataService;


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


        public List<WageBalance> BalanceWages()
        {

            var earningList = _DataService.GetEarningsList(); //DataService returns a List<Earning> list
            var deductionList = _DataService.GetDeductionList(); //DataService returns a List<Deduction>
            int i = 0;

            foreach (Earning e in earningList)
            {

    // Sum all earnings...

            var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i].ExtraShiftAmount + earningList[i].BroughtForwardAmount;

    // Sum all deductions...

            var totalDeductions = deductionList[i].AdvanceAmount + deductionList[i].CarriedForwardAmount + deductionList[i].DeathDonationAmount + deductionList[i].EPFAmount + deductionList[i].FineAmount + deductionList[i].InsuranceInstalmentAmount + deductionList[i].LoanInstalmentAmount + deductionList[i].MealsAmount + deductionList[i].OtherDeductionAmount + deductionList[i].UniformInstalmentAmount + deductionList[i].WelfareAmount;
            var employeeID = earningList[i].EmployeeID;
            var netEarning = totalEarnings - totalDeductions;

            WageInfo wageInfo = new WageInfo();

            wageInfo.WageBalance.EmployeeID = employeeID;
            wageInfo.WageBalance.NetWage = decimal.Truncate(netEarning / 100) * 100;
            wageInfo.WageBalance.CarriedForwardAmount = netEarning - wageInfo.WageBalance.NetWage;

            _WageInfo.WageBalanceList.Add(wageInfo.WageBalance);
            i += 1;
        }

        _DataService.BalanceWages(_WageInfo.WageBalanceList);

        return _WageInfo.WageBalanceList;
    }
}


class WageInfo
{
    public DateTime WagePeriodStartDate { get; set; }
    public DateTime WagePeriodEndDate { get; set; } 
    public string Reference { get; set; }
    public Deduction Deduction = new Deduction();
    public Earning Earning = new Earning();
    public WageBalance WageBalance = new WageBalance();
    public List<WageBalance> WageBalanceList = new List<WageBalance>();

    public WageInfo()
    {
    }

}

public class WageBalance
{
    public int EmployeeID {get;set;}
    public string EmployeeName { get; set; }
    public decimal NetWage { get; set; }
    public decimal CarriedForwardAmount { get; set; }
}
\$\endgroup\$
2
  • \$\begingroup\$ Does Deduction have employeeID also? And how do you know GetEarningsList() and GetDeductionList() return matching elements? How do you know they will stay that way? \$\endgroup\$ May 22, 2014 at 6:39
  • \$\begingroup\$ @abuzittin gillifirca, Yes both Deduction and Earning have employeeID. \$\endgroup\$
    – CAD
    May 22, 2014 at 10:55

3 Answers 3

6
\$\begingroup\$

It looks like you have a pair of lists which you iterate through based on the elements contained in the earningList. This is potentially dangerous without validation. When the earningList is longer than deductionList, you will run into an IndexOutOfRangeException.

var earningList = _DataService.GetEarningsList(); //DataService returns a List<Earning> list
var deductionList = _DataService.GetDeductionList(); //DataService returns a List<Deduction>

int i = 0;
foreach (Earning e in earningList)
{
    //...
    i += 1;
}

So in order to prevent that to happen, detect it in advance and handle it.

if (earningList.Count != deductionList.Count)
{
    //handle exception...
}

Since you dealing with two identical list, you can zip them together with Enumerable.Zip and loop through both at same time

var pairedList = earningList.Zip(deductionList, (e,d) => new { Earning = e, Deduction = d });
foreach(var pair in pairedList)
{
    //calculate wage with pair.Earning and pair.Deduction...
}

If I zip two list as above, is it possible to show these two lists in two DataGridViews?

Assume that we lost the reference to earningList and deductionList somehow and that you want to extract the original lists back from the pairedList to display each separately on DataGridView, it can be done with IEnumerable.Select :

//As dgv's DataSource doesn't take IEnumerable as valid source,
//we need to convert it to one of the interface it accepts : IList
dgvEarning.DataSource = pairedList.Select(p => p.Earning).ToList();
dgvDeduction.DataSource = pairedList.Select(p => p.Deduction).ToList();
\$\endgroup\$
6
  • \$\begingroup\$ If I zip two list as above, is it possible to show these two lists in two DataGridViews? \$\endgroup\$
    – CAD
    May 23, 2014 at 3:30
  • \$\begingroup\$ I've updated my answer for your question. \$\endgroup\$
    – Xiaoy312
    May 23, 2014 at 17:35
  • \$\begingroup\$ Could we link more than two lists in this way? I would like getting a WageInfo object returned from DataService rather than getting Earning, decuction etc. as these are composite objects. But the problem is how to access those private fields of WageInfo? [private List<Earning>;] \$\endgroup\$
    – CAD
    May 25, 2014 at 15:27
  • \$\begingroup\$ @Chathuranga You can expand the existing list by zipping it against the list of WageInfo : pairedList.Zip(anotherList, (p,a) => new { p.Earning, p.Deduction, Something = a }); \$\endgroup\$
    – Xiaoy312
    May 26, 2014 at 17:48
  • \$\begingroup\$ However, if you have more than just that, use a Select to clue them clue together : `Enumerable.Range(0, firstList.Count).Select(i => new { First = firstList[i]; Second = secondList[i]; LastA = lastList[i].PropertyA }); \$\endgroup\$
    – Xiaoy312
    May 26, 2014 at 17:52
13
\$\begingroup\$

Formatting

formatting inside your foreach should be indented

This:

        foreach (Earning e in earningList)
        {

// Sum all earnings...

        var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i].ExtraShiftAmount + earningList[i].BroughtForwardAmount;

Should look like this

        foreach (Earning e in earningList)
        {
            // Sum all earnings...
            var totalEarnings = earningList[i].AmountForEPF + earningList[i].OverTimeAmount + earningList[i].IncentiveAllowance + earningList[i].SpecialAllowance + earningList[i].OtherAllowance + earningList[i].ExtraShiftAmount + earningList[i].BroughtForwardAmount;
            ...
        }

When you are adding things together (either in concatenation or variable addition) you can multi-line the expression because C# is awesome and isn't NewLine Terminated, so this:

  var totalDeductions = deductionList[i].AdvanceAmount + deductionList[i].CarriedForwardAmount + deductionList[i].DeathDonationAmount + deductionList[i].EPFAmount + deductionList[i].FineAmount + deductionList[i].InsuranceInstalmentAmount + deductionList[i].LoanInstalmentAmount + deductionList[i].MealsAmount + deductionList[i].OtherDeductionAmount + deductionList[i].UniformInstalmentAmount + deductionList[i].WelfareAmount;

can be easier read when written like this:

  var totalDeductions = deductionList[i].AdvanceAmount +
      deductionList[i].CarriedForwardAmount + 
      deductionList[i].DeathDonationAmount + 
      deductionList[i].EPFAmount + 
      deductionList[i].FineAmount + 
      deductionList[i].InsuranceInstalmentAmount + 
      deductionList[i].LoanInstalmentAmount + 
      deductionList[i].MealsAmount + 
      deductionList[i].OtherDeductionAmount + 
      deductionList[i].UniformInstalmentAmount + 
      deductionList[i].WelfareAmount;

I am looking into making this easier to do as well, that just looks messy


Organization

The more that I look at your code, there are more things that I would change.

your Deduction and Earning objects are created and maintained by your WageInfo class so really you should be using a wageInfoList and each item in that list will have one Deduction object and one Earning object

This means that the WageInfo object should have all the information to hand off to the WageManager. The WageManager shouldn't directly see the Deduction or Earning objects that live in the WageInfo Object that is called by the WageManager.

\$\endgroup\$
1
  • \$\begingroup\$ As for the readability with adding array elements, I think it would read better if the operator was placed in front. \$\endgroup\$
    – Pimgd
    Dec 18, 2014 at 15:38
10
\$\begingroup\$

It seems to me that you're hard-coding far too much here.

IMO, this type of task should be heavily database driven. For example, each item of earning and each deduction should have an amount, and a category. The category should come from a table in the database. So, you'd end up with one table of categories of deductions including the ones you have above (welfare, insurance, EPF, fine, meals, etc.) That table would probably have its permissions set so most normal users could only select from among the existing categories, but someone with administrative rights could add new ones. Of course, earnings would be pretty much the same way.

The code would then query the database for all deductions that apply to a particular employee during a specified time period, and add them together to get that employee's deductions for a pay period, total for the year, or whatever. This lets you (among other things) add or remove deductions without modifying the code.

The simple fact is that it's vain to even hope that when you write the code you've thought of every possible category of deduction somebody might want or need. Hard-coding leaves only two real choices: either resign yourself to modifying the code constantly forever, or force users to twist and stretch everything that arises to fit the categories that are currently available. In practice, such systems typically end up with some ugly combination of both.

Moving the categories to a database probably won't eliminate that--if somebody needs an administrator to add a category means somebody will inevitably find it more work than it's worth at least once in a while. Even if you let normal users add categories, some will still be too lazy to add a category sometimes1. Nonetheless, with a reasonable system it takes only fairly minimal work on the part of a few people to keep the system working fairly smoothly, at least as a rule.


1. For comparison, I'd cite the tags system on this site (and most others like it). Yes, moderators need to edit tags on questions semi-regularly (hi @Jamal) -- but at least being an entry in a database table means they have the tools available to do that editing, without having to ask somebody at Stack Exchange to re-compile the code every time they need a new tag.

\$\endgroup\$
2
  • \$\begingroup\$ I understand the importance of a category table. But I don't understand whats in my code far too much hard coding. All deductions and earnings are calculate by some other algorithm and saved in a table. Then those figures are accessed by methods like _DataService.GetEarningsList() in the above code. \$\endgroup\$
    – CAD
    May 21, 2014 at 16:49
  • \$\begingroup\$ I was referring to things like AmountForEPF, OverTimeAmount, IncentiveAllowance, SpecialAllowance, and so on. \$\endgroup\$ May 21, 2014 at 17:15

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.