Link to home
Start Free TrialLog in
Avatar of jxharding

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);   

Open in new window

How would you have done it?
Avatar of Walter Ritzel
Walter Ritzel
Flag of Brazil image

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.
Avatar of jxharding


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
Avatar of it_saige
Flag of United States of America image

Link to home
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
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.
@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);


Open in new window

Produces the following output -User generated imageYou 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.

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

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#.