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?
jxhardingAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

Walter RitzelSenior Software EngineerCommented:
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.
0
jxhardingAuthor Commented:
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
0
it_saigeDeveloperCommented:
Without knowing the actual layout of your object(s); perhaps something like this:
static class Extensions
{
	public static IEnumerable<InvoiceSummary> GetInvoiceSummaries(this IEnumerable<Invoice> data)
	{
		return (from invoice in data
			   select new InvoiceSummary() 
			   { 
				   Id = invoice.Id, 
				   LineItemAvgCost = (double)invoice.LineItems.Sum(x => x.Count) / (double)invoice.LineItems.Sum(x => x.Cost), 
				   SalesPercentage = ((double)invoice.Items / (double)invoice.Visits) * 100 
			   });
	}
}

Open in new window

Example usage (not really using any real data):
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace EE_Q28699687
{
	class Program
	{
		static List<Invoice> InvoiceData = new List<Invoice>();
		static void Main(string[] args)
		{
			for (int i = 1; i < 11; i++)
			{
				Invoice invoice = new Invoice() { Id = i, Items = i * 2, Visits = (i / 2 != 0) ? i / 2 : 1 };
				for (int j = 1; j < i + 6; j++)
					invoice.LineItems.Add(new LineItem() { Count = i + 6, Cost = (i * 6m / .33m) });
				InvoiceData.Add(invoice);
			}

			foreach (var summary in InvoiceData.GetInvoiceSummaries())
				Console.WriteLine(summary);

			Console.ReadLine();
		}
	}

	static class Extensions
	{
		public static IEnumerable<InvoiceSummary> GetInvoiceSummaries(this IEnumerable<Invoice> data)
		{
			return (from invoice in data
				   select new InvoiceSummary() 
				   { 
					   Id = invoice.Id, 
					   LineItemAvgCost = (double)invoice.LineItems.Sum(x => x.Count) / (double)invoice.LineItems.Sum(x => x.Cost), 
					   SalesPercentage = ((double)invoice.Items / (double)invoice.Visits) * 100 
				   });
		}
	}

	class LineItem
	{
		public int Count { get; set; }
		public decimal Cost { get; set; }
	}

	class Invoice
	{
		public int Id { get; set; }
		public int Items { get; set; }
		public int Visits { get; set; }

		private List<LineItem> fLineItems;
		public List<LineItem> LineItems
		{
			get
			{
				if (fLineItems == null)
					fLineItems = new List<LineItem>();
				return fLineItems;
			}
			set { fLineItems = value; }
		}
	}

	class InvoiceSummary
	{
		public int Id { get; set; }
		public double SalesPercentage { get; set; }
		public double LineItemAvgCost { get; set; }
		public override string ToString()
		{
			return string.Format("ID: {0}; SalesPercentage: {1}; LineItemAvgCost: {2}", Id, SalesPercentage, LineItemAvgCost);
		}
	}
}

Open in new window

Produces the following output -Capture.JPG
1

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
Cloud Class® Course: Certified Penetration Testing

This CPTE Certified Penetration Testing Engineer course covers everything you need to know about becoming a Certified Penetration Testing Engineer. Career Path: Professional roles include Ethical Hackers, Security Consultants, System Administrators, and Chief Security Officers.

Jacques Bourgeois (James Burger)PresidentCommented:
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.
0
it_saigeDeveloperCommented:
@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 -Capture.JPGYou 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-
0
jxhardingAuthor Commented:
This is awesome thank you so much. This provided the insight i was looking for.
Thank you again
0
Jacques Bourgeois (James Burger)PresidentCommented:
@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#.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
.NET Programming

From novice to tech pro — start learning today.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.