asked on

IEnumerable - Loop all child records (LineItems) per Invoice - example included - architect said inefficient

The goal of this question is to pick up proper coding skills from a professional and become a better developer

An architect said my code was not efficient.

The idea is to loop list of InvoiceData,
each Invoice has line items, then I must perform a calculation based on the LineItems per Invoice.

        public IEnumerable<InvoiceSummary> GetInvoiceSummary()
        {
            List<InvoiceSummary> lis = new List<InvoiceSummary>();
            foreach(Invoice t  in InvoiceData)
            {
                InvoiceSummary InS = new InvoiceSummary();
                InS.Id = t.Id;
                InS.SalesPercentage = ((double)t.Items / (double)t.Visits) * 100;
                int LineItemCount = 0;
                int LineItemCost = 0;
                foreach (LineItem d in t.LineItems)
                {
                    LineItemCount += d.LineItemCount;
                    LineItemCost += d.LineItemCost;
                }
                InS.LineItemAvgCost = ((double)LineItemCount / (double)LineItemCost);   
                lis.Add(InS);
            }
        }

Open in new window

How would you have done it?

Walter Ritzel

I would have used a SQL to do that, if you are not using Entity Framework. If EF is being used, I would have written a LINQ query.

jxharding

ASKER

OK can you give and hints on the LINQ please, I tried to get the sum of the 2 columns from LIneItems but could not succeed, and reverted to above methods

ASKER CERTIFIED SOLUTION

it_saige

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

Jacques Bourgeois (James Burger)

Why do you cast all your int to double?

You do not have to do that, the result of a division of 2 numbers is automatically a double, no matter the type of numeric value you have.

You are also declaring 2 new variables LineItemCount and LineItemCost each time you go through the main loop. I would rather declare these variable once, before entering the loop, and reset them to zero before entering the inner loop.

it_saige

@James - You have to cast because straight integer division removes the remainder. Proof of concept.

using System;

namespace EE_Q28699687
{
	class Program
	{
		static void Main(string[] args)
		{
			Console.WriteLine(1 / 2);
			Console.WriteLine((double)1 / (double)2);

			Console.ReadLine();
		}
	}
}

Open in new window

Produces the following output -

You don't *have* to cast both sides (because of widening conversion) but I usually do it just so that anyone coming behind me understands my intent. Also casting after the division will not reinstate the remainder; e.g.

Console.WriteLine((double)(1 / 2));

Open in new window

Still produces an output of zero.

-saige-

jxharding

ASKER

This is awesome thank you so much. This provided the insight i was looking for.
Thank you again

Jacques Bourgeois (James Burger)

@it_saige

Thanks for the info, this is one of these little differences between VB and C# that I was not aware of. And my answer shows that I do a lot more VB than C# :-).

Strange thing. If you look at the type returned by 1/2 in a Watch window, C# it returns an int while in VB returns a Double.

In vb, you need to use the inverse divisor, 1\2, in order to get what you have with the regular divisor in C#.