2

I have some weird behaviour with a foreach-loop:

 IEnumerable<Compound> loadedCompounds;
 ...
 // Loop through the peaks.
 foreach (GCPeak p in peaks)
 {
     // Loop through the compounds.
     foreach (Compound c in loadedCompounds)
     {
         if (c.IsInRange(p) && c.SignalType == p.SignalType)
         {
             c.AddPeak(p);
         }  
     }
 }

So what I'd like to do: Loop through all the GCPeaks (it is a class) and sort them to their corresponding compounds.

AddPeak just adds the GCPeak to a SortedList. Code compiles and runs without exceptions, but the problem is:

After c.AddPeak(p) the SortedList in c contains the GCPeak (checked with Debugger), while the SortedLists in loadedCompounds remains empty.

I am quite confused with this bug I produced:

  1. What is the reason for this behavior? Both Compound and GCPeak are classes so I'd expect references and not copies of my objects and my code to work.
  2. How to do what I'd like to do properly?

EDIT:

This is how I obtain the IEnumarables (The whole thing is coming from an XML file - LINQ to XML). Compounds are obtained basically the same way.

 IEnumerable<GCPeak> peaksFromSignal = from p in signal.Descendants("IntegrationResults")
                                        select new GCPeak()
                                        {
                                            SignalType = signaltype,
                                            RunInformation = runInformation,
                                            RetentionTime = XmlConvert.ToDouble(p.Element("RetTime").Value),
                                            PeakArea = XmlConvert.ToDouble(p.Element("Area").Value),
                                        };

Thanks!

AndreasN
  • 67
  • 9
  • Is `Compound` a struct or a class? If you could provide enough of your code for us to easily reproduce the issue, that would be a big help. – 15ee8f99-57ff-4f92-890c-b56153 Mar 27 '17 at 20:03
  • 5
    Because `loadedCompounds` is an enumerable and probably not a real list. Try to call `loadedCompounds = loadedCompounds.ToList()` before the first loop. – Kalten Mar 27 '17 at 20:07
  • Both Compound and GCPeak are classes. – AndreasN Mar 27 '17 at 20:08
  • Kalten makes a good point. Make sure that you're enumerating the same *actual instances* of `Compound` in both loops -- the one you showed us, and the one where you find the peaks missing. My own question was rather silly I'm afraid. – 15ee8f99-57ff-4f92-890c-b56153 Mar 27 '17 at 20:09
  • How is the `loadedCompounds` iterator implemented? If it's making new `Compound` objects every time it's iterated all data would be lost. – dcg Mar 27 '17 at 20:11
  • I have added the code how I obtain the iterator above. I will try changing IEnumerable to list: I just thought that IEnumerable is the most generic way of implementation... – AndreasN Mar 27 '17 at 20:42

2 Answers2

1

An IEnumerable won't hold a hard reference to your list. This causes two potential problems for you.

1) What you are enumerating might not be there anymore (for example if you were enumerating a list of facebook posts using a lazy technique like IEnumerable etc, but your connection to facebook for is closed, then it may evaluate to an empty enumerable. The same would occur if you were doing an IEnumerable over a database collection but that DB connection was closed etc.

2) Using an enumerable like that could lead you later to or previously to that to do a multiple enumeration which can have issues. Resharper typically warns against this (to prevent unintended consequences). See here for more info: Handling warning for possible multiple enumeration of IEnumerable

What you can do to debug your situation would be to use the LINQ extension of .toList() to force early evaluation of your IEnumerable. This will let you see what is in the IEnumerable easier and will let you follow this through your code. Do note that doing toList() does have performance implications as compared to a lazy reference like you have currently but it will force a hard reference earlier and help you debug your scenario and will avoid scenarios mentioned above causing challenges for you.

Community
  • 1
  • 1
Dessus
  • 2,147
  • 1
  • 14
  • 24
  • Thanks, this helped a lot. When looking back at how I get my IEnumerable: I am right to assume that upon each evaluaton of the IEnumarble, it was newly created from by LINQ to XML from the XML file? – AndreasN Mar 27 '17 at 20:59
  • If its an XML file, is the filestream open when evaluating the IEnumerable? – Dessus Mar 27 '17 at 21:06
  • It's coming from an XDocument.Load("filename"). That XDocument, however, is still in memory. – AndreasN Mar 27 '17 at 21:21
0

Thanks for your comments.

Indeed converting my loadedCompounds to a List<> worked.

Lesson learned: Be careful with IEnumerable.

EDIT As requested, I am adding the implementation of AddPeak:

public void AddPeak(GCPeak peak)
{
    if (peak != null)
    {
        peaks.Add(peak.RunInformation.InjectionDateTime, peak);
    }
}

RunInformation is a struct.

AndreasN
  • 67
  • 9
  • This is strange behavior because XDocument isn't IDisposable. Can you include the c.AddPeak defintion? I bet the answer why is in there – Dessus Mar 28 '17 at 00:19
  • I don't think the lesson here is learnt. You don't understand why toList() solved your issues. Without that you likely will either use toList() too much or repeat the mistake. IEnumerable should work correctly in your case unless you have a mistake (that .toList() is hiding). I suspect you are modifying collections in a way that you don't intend. – Dessus Mar 28 '17 at 00:21
  • I appreciate going this through further with me. I have added the AddPeak method. – AndreasN Mar 28 '17 at 08:25
  • This looks dangerous to me, as you are modifying the outside foreach loops collection with the inner loop. This might have unintended consequences. I have had pain doing this myself before. It seems like you are trying to simulate recursion with two foreach loops. Perhaps recursion might work better? I believe it would be faster as recursion is done on the cpu stack from my understanding – Dessus Mar 29 '17 at 19:14