3
\$\begingroup\$

Subject

I have to iterate through a collection of objects (in this case IGrouping<string, SupplyDemand>). So I could do this easyly using a query with LINQ.

Code

    public IEnumerable<string> getParentPegs(IGrouping<string, SupplyDemand> data)
    {
        IEnumerable<string> query =
            from d in data
            where d.Source.ToLower() == "make"
            select d.Part;

        return query;
    }

Problem

I'm using this function inside a loop in a VSTO Excel project and it's pretty slow, besides it stops running (memory leak). So I came up with the following code:

Improved Code

    public List<string> getParentPegs(IGrouping<string, SupplyDemand> data)
    {
        List<string> result = new List<string>();

        var enumerator = data.GetEnumerator();
        while (enumerator.MoveNext())
        {
            SupplyDemand obj = enumerator.Current;
            if (obj.Source.ToLower() == "make")
            {
                result.Add(obj.Peg);
            }
        }

        return result;
    }

Question

Is there any other way to improve this?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ How are you using the getParentPegs method? The original code returns a lazily evaluated expression and not an actual collection, so if in your code you do this: var pegs = getParentPegs(....); int count = pegs.Count(); foreach(var peg in pegs){ // Doing something else here } Will cause your expression to be evaluated twice, whereas the improved code will only evaluate it once. You can overcome this by forcing it to evaluate and store it in a list by calling the .ToList() method: var pegs = getParentPegs(...).ToList(); Or return a List within getParentPegs. \$\endgroup\$
    – Makotosan
    Feb 27, 2012 at 16:11
  • \$\begingroup\$ Makotosan; I'm using it inside a loop, that's what I've been trying to optimize (archiving all parent pegs for a component). Yes, you're right about evaluating the expression twice, so I came up with the second code. So mainly, this question is about the linq performance against a simple iteration. \$\endgroup\$
    – Eder
    Feb 27, 2012 at 18:55

2 Answers 2

3
\$\begingroup\$

You can do the same with just a foreach loop instead of reading the enumerator.

You can use the String.Compare method to do a case insensetive compare instead of creating a lowercase version of each string that you compare.

Improved improved code:

public List<string> getParentPegs(IGrouping<string, SupplyDemand> data) {

  List<string> result = new List<string>();

  foreach (SupplyDemand obj in data) {
    if (String.Compare(obj.Source, "make", true) == 0) {
      result.Add(obj.Peg);
    }
  }

  return result;
}

Note: In your first code you are getting the property Part, but in the second you are getting the property Peg instead.

\$\endgroup\$
2
\$\begingroup\$

how about this :

 public IEnumerable<string> GetParentPegs(IGrouping<string, SupplyDemand> data)
    {
        foreach(var d in data)
             if(d.Source.ToLower() == "make") yield return d.Part;

    }
\$\endgroup\$
3
  • \$\begingroup\$ Excellent!, But how come that this is faster than my second example? \$\endgroup\$
    – Eder
    Feb 25, 2012 at 22:43
  • \$\begingroup\$ It actually does the same thing but it's a bit more readable in my opinion \$\endgroup\$ Feb 25, 2012 at 23:02
  • 1
    \$\begingroup\$ It might be faster if you are not evaluating the result of GetParentPegs immediately, since the loop in GetParentPegs will only be executed when the IEnumerable<string> is enumerated. \$\endgroup\$ Feb 26, 2012 at 0:18

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.