1
\$\begingroup\$

I have multiple repositories that contain userId, paidDate and amount attributes like Bills(userId, paidDate, amount) Shoppings(userId, paidDate, amount) etc. I want to get sum of amounts and group them by userId and get something like this {user1: {amount1, amount2, amount3 ..}, user2: {amount4, ..}}

I created a expenseDetails model that contains all of repositories above.

public class ExpenseDetails
{
    public IEnumerable<ApplicationUser> ApplicationUsers { get; set; }
    public IEnumerable<Bill> Bills { get; set; }
    public IEnumerable<Shopping> Shoppings { get; set; }
    public IEnumerable<Rent> Rents { get; set; }
}

And I created a helper method for calculate the sum of amounts with looping every model in expenseDetails.

public static Dictionary<string, List<double>> UsersExpense(ExpenseDetails expenseDetails)
    {
        Dictionary<string, List<double>> TotalCosts = new Dictionary<string, List<double>>();
        List<double> Total = new List<double>();
        double sum = 0;

        foreach (var user in expenseDetails.ApplicationUsers)
        {
            foreach (var item in expenseDetails.Shoppings)
            {
                if (item.ApplicationUserId == user.Id)
                {
                    if (Convert.ToInt32(item.PaidDate.Split("/")[1]) == DateTime.Now.Month)
                    {
                        sum = item.Amount + sum;
                    }
                }
            }

            Total.Add(sum);
            sum = 0;

            foreach (var item in expenseDetails.Bills)
            {
                if (item.ApplicationUserId == user.Id)
                {
                    if (Convert.ToInt32(item.PaidDate.Split("/")[1]) == DateTime.Now.Month)
                    {
                        sum = item.Amount + sum;
                    }
                }
            }

            Total.Add(sum);
            sum = 0;

            foreach (var item in expenseDetails.Rents)
            {
                if (item.ApplicationUserId == user.Id)
                {
                    if (Convert.ToInt32(item.PaidDate.Split("/")[1]) == DateTime.Now.Month)
                    {
                        sum = item.Amount + sum;
                    }
                }
            }

            Total.Add(sum);
            sum = 0;

            TotalCosts.Add(user.Id, Total.ToList());
            Total.Clear();
        }

        return TotalCosts;
    }

But this is not a good one for programming. How can I achive this with best practice? I am using repository pattern by the way.

\$\endgroup\$
3
\$\begingroup\$

Some quick remarks:

  • Why isn't PaidDate a DateTime?
  • What is a "Shopping"? (I realize that often naming things is hard, but be careful when inventing new English words that do not adequately describe their contents, because the next person to maintain this code will find it harder to do so when they have to first decipher various terms.)
  • Use descriptive names: item is way too vague.
  • TotalCosts and Total should be camelCased.
  • sum = item.Amount + sum; can be shortened to sum += item.Amount;.
  • Don't do if (item.ApplicationUserId == user.Id), instead use LINQ to extract the relevant records, e.g. expenseDetails.Shoppings.Where(x => x.ApplicationUserId == user.Id). And if PaidDate was a DateTime, you could easily also include that in the LINQ query and immediately calculate the sum.
  • Why do you do Total.ToList()? How can it be anything else?

But all of that is IMHO just plugging some small holes, while ignoring the massive dam breach elsewhere: surely this logic could be done easily in one SQL query?

On a related note: is there even a point to have a List<double> Total? I wouldn't be surprised if all you did with those is add them up.

And if you're not adding them up and always assume that the first item is the sum of Shoppings etc. and use this to display them elsewhere: please don't. In such cases always use the key-value pair structure, so you'd know that value X is definitely the sum of the Shoppings.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your advices BCdotWEB \$\endgroup\$ – abdllhcay Oct 5 at 9:10

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.