7

Seen a weird bit of behaviour in some C# code that I'm at a loss to explain. Could be I'm missing an important bit of understanding, so hoping someone out there can switch on the light for me.

Got a block of code that looks like this:

    IEnumberable<myObject> objects = GetObjectsFromApiCall();

    for (int i = 0; i < objects.Count(); i++)
        {
            if (String.IsNullOrEmpty(objects.ElementAt(i).SubObject.Title))
            {
                SubObject sub = GetSubObjectFromDatabase((long)objects.ElementAt(i).SubObject.Id);
                if (sub != null)
                {
                    objects.ElementAt(i).SubObject.Title = sub.Title;
                }
            }
        }

When you step through it, everything about this code seems to work properly. The "objects" collection is populated as expected. "sub" is fetched as collected and has a full set of expected properties, including a populated Title property. No errors are thrown during execution.

But ... the SubObject.Title property (which just has standard get; set; code) that exists in each Object stubbornly remains empty.

I'm at a loss. Anyone explain what's going on?

EDIT: For those who suggested I shouldn't use a for loop and ElementAt, I started with a foreach loop but thought it might be the source of the problem because it was fetching a new SubObject each time round. Fixed now, thanks to your help, and the ForEach restored.

Cheers, Matt

Bob Tway
  • 9,301
  • 17
  • 80
  • 162
  • 4
    There you go: [Updating an item property within IEnumerable but the property doesn't stay set?](http://stackoverflow.com/a/9104212/93732) – Ahmet Kakıcı Mar 08 '13 at 12:58
  • This code has the potential to be so slow it is not even funny. – ChaosPandion Mar 08 '13 at 12:58
  • Could you copy/paste the *actual* code you have, not something that *looks* like the *actual* code? – ken2k Mar 08 '13 at 13:00
  • It works, but you might be comparing different copies of the data. Can you confirm that both `myObject` and the type of the `SubObject` property (or field) are `class` types, as opposed to `struct` (or `interface`) types? If the method call `GetObjectsFromApiCall()` generates a new copy of the data each time it is called, you're only modifying the copy. The "original" data will be unchanged in that case. – Jeppe Stig Nielsen Mar 08 '13 at 13:15
  • @ChaosPandion I assume you meant that in the spirit of helpful co-operation. I know it could be slow, but it won't end up handling more than 10 objects at a time. – Bob Tway Mar 08 '13 at 13:53
  • @MattThrower - I really did although my delivery was terrible. My goal was to comment so those who answer will remember to add a special note regarding the code. – ChaosPandion Mar 08 '13 at 19:28

5 Answers5

5

I would fix it this way:

var objects = GetObjectsFromApiCall().ToList();

Then you could keep the loop as is (it works), or optimize it a bit using foreach and some Linq as suggested by other answers, but it does not really matter: the problem was that you attempted to change an element on an IEnumerator<> as explained in this question pointed by @Ahmet Kakıcı.

Community
  • 1
  • 1
Larry
  • 17,605
  • 9
  • 77
  • 106
  • 1
    -1 This is actually wrong. There is no problem in modifying elements returned by an `IEnumerable`. Actually, when you're using `foreach` after a `ToList()`, you're precisely using an `IEnumerable`, as `List` implements `IEnumerable`. One problem could be the *deferred execution* of a DB query, but it's certainly not because of the simple presence of the `IEnumerable` interface... – ken2k Mar 08 '13 at 15:13
  • You are right, I read too fast... The problem is not the IEnumerable but the way it is implemented. That's why using ToList() makes sense. Thanks for the clarification. – Larry Mar 08 '13 at 16:01
  • @ken2k Correct. See my answer for examples where I modify the items returned by an `IEnumerable`. – Jeppe Stig Nielsen Mar 08 '13 at 16:13
2

Try this

List<myObject> objects = GetObjectsFromApiCall().ToList();

foreach(var obj in objects.Where(o => string.IsNullOrEmpty(objects.SubObject.Title)).ToList())
{
    var subObject = GetSubObjectFromDatabase(obj.SubObject.Id);
    if(subObject == null) continue;

    obj.SubObject.Title = subObject.Title;
}
Dustin Kingen
  • 20,677
  • 7
  • 52
  • 92
1

First of all, you should not use ElementAt() for this kind of code, use

foreach (var o in objects)
{
    if (string.IsNullOrEmpty(o.SubObject.Title))
    {
        o.SubObject.Title = ...;
    }
}

Also you should note that if your method returns a dynamic IEnumerable then every time you call objects.Something() the API is called again and a fresh copy is retrieved. If this is the case, you should copy the enumerable into a list using .ToList() method.

There is also a way of not putting a copy in the list - by creating a dynamic enumerator like this:

objects = objects.Select(o =>
{
    if (string.IsNullOrEmpty(o.SubObject.Title))
    {
        o.SubObject.Title = ...;
    }
    return o;
});

As for the value not being set correctly (if previous things did not help) - try adding a throw new Exception(value) in the setter for Title property - see if that is being called with the correct value.

Knaģis
  • 20,827
  • 7
  • 66
  • 80
  • "First of all, you should not use ElementAt() for this kind of code." Why? – Kenneth K. Mar 08 '13 at 13:43
  • .NET will enumerate every time using `Enumerator.MoveNext()` for `i` times to get the value. it is way slower than `list[i]` approach. – Knaģis Mar 08 '13 at 13:55
1

I guest the function GetObjectsFromApiCall looks like following:

public IEnumberable<myObject> GetObjectsFromApiCall(){
    for(var i = 0; i < 10; i++)
    {
         yield return new myObject();
    }
}

If I'm right, every time you call objects.ElementAt(i) function to get the object, you will get a new object by "yield return new myObject()".

fengyj
  • 75
  • 1
  • 8
1

But how do you check if the Title property is changed? Do you call GetObjectsFromApiCall() again? Or do you foreach through the same objects instance again?

An IEnumerable instance may create and yield new objects each time it is "enumerated". So here's a simple example for illustration. For the example, define:

class SomeObject
{
    public string Title { get; set; }
}

Then we will consider two types of "source", first an array, and then an iterator block defined like this:

  static IEnumerable<SomeObject> GetSomeSequence()
  {
      yield return new SomeObject { Title = "Alpha", };
      yield return new SomeObject { Title = "Beta", };
      yield return new SomeObject { Title = "Gamma", };
  }

Then test it this way:

  static void Main()
  {
      IEnumerable<SomeObject> thingsToModify;

      // set source to an array
      thingsToModify = new[] { new SomeObject { Title = "Alpha", }, new SomeObject { Title = "Beta", }, new SomeObject { Title = "Gamma", }, };

      foreach (var t in thingsToModify)
          Console.WriteLine(t.Title);

      foreach (var t in thingsToModify)
          t.Title = "Changed!";

      foreach (var t in thingsToModify)
          Console.WriteLine(t.Title);    // OK, modified


      // set source to something which yields new object each time a new GetEnumerator() call is made
      thingsToModify = GetSomeSequence();

      foreach (var t in thingsToModify)
          Console.WriteLine(t.Title);

      foreach (var t in thingsToModify)
          t.Title = "Changed!";          // no-one keeps these modified objects

      foreach (var t in thingsToModify)
          Console.WriteLine(t.Title);    // new objects, titles not modified

  }

Conclusion: It's perfectly possible to modify the state of a mutable object which belongs to the source we're iterating over. But some types of IEnumerable sources yield new copies of the data each time they are called, and then it's useless to make modifications to the copy.

Jeppe Stig Nielsen
  • 60,409
  • 11
  • 110
  • 181