14 Replies - 816 Views - Last Post: 17 March 2011 - 08:52 AM Rate Topic: -----

#1 kstr  Icon User is offline

  • D.I.C Head

Reputation: 4
  • View blog
  • Posts: 60
  • Joined: 19-October 09

Best practice

Posted 16 March 2011 - 11:16 AM

How can i do this better /quicker:


  List<string> ids = new List<string>();
                foreach (Product pr in products)
                {
                    foreach (ProductVariant pv in pr.ProductVariants)
                    {
                        ids.Add(pv.ManufacturerPartNumber);
                    }
                }



Is This A Good Question/Topic? 0
  • +

Replies To: Best practice

#2 tlhIn`toq  Icon User is offline

  • Please show what you have already tried when asking a question.
  • member icon

Reputation: 5481
  • View blog
  • Posts: 11,762
  • Joined: 02-June 10

Re: Best practice

Posted 16 March 2011 - 11:22 AM

You might be able to do it in a single line or two in Linq.
But I don't think that would be any 'better' or 'faster' since it is really going to have to be a couple loops anyway, just hidden away in the Framework where the coder doesn't see it.

What makes you think there is something wrong with the way you are doing it now?
Was This Post Helpful? 0
  • +
  • -

#3 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4469
  • View blog
  • Posts: 7,780
  • Joined: 08-June 10

Re: Best practice

Posted 16 March 2011 - 11:26 AM

Well, you could do it with LINQ:
var ids = products.Select(pr => pr.ManufacturerPartNumber).ToList();


Assuming you're using .NET 3.5 or higher, and you have add using System.Linq; to the using directives.
Was This Post Helpful? 2
  • +
  • -

#4 kstr  Icon User is offline

  • D.I.C Head

Reputation: 4
  • View blog
  • Posts: 60
  • Joined: 19-October 09

Re: Best practice

Posted 16 March 2011 - 12:41 PM

View PosttlhIn`toq, on 16 March 2011 - 11:22 AM, said:

You might be able to do it in a single line or two in Linq.
But I don't think that would be any 'better' or 'faster' since it is really going to have to be a couple loops anyway, just hidden away in the Framework where the coder doesn't see it.

What makes you think there is something wrong with the way you are doing it now?



Well while i was writing it, i got an eery feeling about it :dozingoff:

It seemed not 'best practice' to iterate in a nested way like that.
Was This Post Helpful? 0
  • +
  • -

#5 tlhIn`toq  Icon User is offline

  • Please show what you have already tried when asking a question.
  • member icon

Reputation: 5481
  • View blog
  • Posts: 11,762
  • Joined: 02-June 10

Re: Best practice

Posted 16 March 2011 - 12:47 PM

That's probably what loops do 99% of the time: Iterate through a list or collection of something.

Nested loops you see allot for either x,y data structures or for hierarchical structures. So you're using it as it was intended.
Was This Post Helpful? 1
  • +
  • -

#6 kstr  Icon User is offline

  • D.I.C Head

Reputation: 4
  • View blog
  • Posts: 60
  • Joined: 19-October 09

Re: Best practice

Posted 16 March 2011 - 12:48 PM

View PostinsertAlias, on 16 March 2011 - 11:26 AM, said:

Well, you could do it with LINQ:
var ids = products.Select(pr => pr.ManufacturerPartNumber).ToList();


Assuming you're using .NET 3.5 or higher, and you have add using System.Linq; to the using directives.


yes, but i still have to select all productvariants and then select among then?
Was This Post Helpful? 0
  • +
  • -

#7 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4469
  • View blog
  • Posts: 7,780
  • Joined: 08-June 10

Re: Best practice

Posted 16 March 2011 - 01:08 PM

Didn't notice the nesting. My LINQ wouldn't work there. But there's nothing explicitly wrong with nested foreach loops. It's O(n2), but that's the best that can be hoped for with that kind of operation.

You still can do this with LINQ though:

var ids = products.SelectMany(x => x.ProductVariants).Select(x => x.ManufacturerPartNumber).ToList();


But the operation is no faster. Select and SelectMany are just obfuscating loops.
Was This Post Helpful? 2
  • +
  • -

#8 kstr  Icon User is offline

  • D.I.C Head

Reputation: 4
  • View blog
  • Posts: 60
  • Joined: 19-October 09

Re: Best practice

Posted 16 March 2011 - 01:28 PM

Ok thanks a lot
Was This Post Helpful? 0
  • +
  • -

#9 ragingben  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 170
  • View blog
  • Posts: 637
  • Joined: 07-October 08

Re: Best practice

Posted 17 March 2011 - 03:30 AM

I like LINQ, but I think that sometimes spreading code over a few lines like the OP code is preferable just because it is so easy to understand for anyone looking at it, especially those migrating from other languages, which in commerical applications is vital.
Was This Post Helpful? 0
  • +
  • -

#10 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2257
  • View blog
  • Posts: 9,445
  • Joined: 29-May 08

Re: Best practice

Posted 17 March 2011 - 03:46 AM

Here you go,
LINQ statements over multiple lines.
var ids = products
          .SelectMany(Product => Product.ProductVariants)
          .Select(ProductVariant => ProductVariant.ManufacturerPartNumber)
          .ToList()


Maybe it is x. that makes it harder to understand.

This post has been edited by AdamSpeight2008: 17 March 2011 - 06:06 AM

Was This Post Helpful? 1
  • +
  • -

#11 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4469
  • View blog
  • Posts: 7,780
  • Joined: 08-June 10

Re: Best practice

Posted 17 March 2011 - 07:43 AM

x is a bad habit of mine. Lots of examples out there used it, so I picked it up as a habit, and it stuck. More descriptive names are better, but it's just what naturally comes to me as I write the query.
Was This Post Helpful? 0
  • +
  • -

#12 eclipsed4utoo  Icon User is offline

  • Not Your Ordinary Programmer
  • member icon

Reputation: 1524
  • View blog
  • Posts: 5,960
  • Joined: 21-March 08

Re: Best practice

Posted 17 March 2011 - 08:21 AM

View PostinsertAlias, on 17 March 2011 - 10:43 AM, said:

x is a bad habit of mine. Lots of examples out there used it, so I picked it up as a habit, and it stuck. More descriptive names are better, but it's just what naturally comes to me as I write the query.


I don't see it as a bad habit. That's like saying using i in a for loop is bad.

Though I don't normally use the same letter in multiple, connected LINQ statements. Like I use t all the time, but in your example, I would have probably used t and s instead of t twice.
Was This Post Helpful? 0
  • +
  • -

#13 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2257
  • View blog
  • Posts: 9,445
  • Joined: 29-May 08

Re: Best practice

Posted 17 March 2011 - 08:27 AM

Using different and proper names is preferred. As x is both lambda function are different and independent variables. They are also are of different types.
Was This Post Helpful? 0
  • +
  • -

#14 tlhIn`toq  Icon User is offline

  • Please show what you have already tried when asking a question.
  • member icon

Reputation: 5481
  • View blog
  • Posts: 11,762
  • Joined: 02-June 10

Re: Best practice

Posted 17 March 2011 - 08:35 AM

Not to mention that most controls have an X property as part of their .Location property. I would just avoid using 'x' all together unless I really meant someObject.Location.X

And really, when it comes to readability/maintainability myFoundResults is a variable I would rather come across in code a year from now than x.

Last I checked there was no shortage of Unicode or ASCII characters and we aren't paying for variable names by the byte.

This post has been edited by tlhIn`toq: 17 March 2011 - 08:35 AM

Was This Post Helpful? 0
  • +
  • -

#15 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4469
  • View blog
  • Posts: 7,780
  • Joined: 08-June 10

Re: Best practice

Posted 17 March 2011 - 08:52 AM

Well, in my defense, x would only be used inside a single-line lambda, and it would be a variable name, not a property. Also, LINQ queries are supposed to be concise. That's a major point of LINQ; brevity. So having a long character variable name as an iterator variable wouldn't make any sense, just like you don't do for(int myIteratorForLoop = 1....

I think a middle ground is better. Single letters aren't bad for simple LINQ queries, but repeating the same one probably is confusing.

This post has been edited by insertAlias: 17 March 2011 - 08:52 AM

Was This Post Helpful? 1
  • +
  • -

Page 1 of 1