You used a foreach, and then you... but... there's a... *twitch*



  • In a 3800 line C# file for a [i]single web control[/i] in our system (all class and instance names changed to protect the profoundly moronic):

    i=0;
    foreach(BusinessObject bO in _instanceVar.BusinessObjectCollection)
    {
    	if(theNumberWeAreLookingFor == bO.SomeNumber)
    	{
    		_instanceVar.BusinessObjectCollection[i].AProperty = something;
    		// some other junk snipped out
    
    	break;
    }
    i++;
    

    }

    It is also worth mentioning that "i" is declared about 200 lines before it is initialized (again) and is not used before or after this block of code.



  • Wow - just - Wow!



  • @djork said:

    In a 3800 line C# file for a [i]single web control[/i] in our system (all class and instance names changed to protect the profoundly moronic):

    i=0;
    foreach(BusinessObject bO in _instanceVar.BusinessObjectCollection)
    {
    if(theNumberWeAreLookingFor == bO.SomeNumber)
    {
    _instanceVar.BusinessObjectCollection[i].AProperty = something;
    // some other junk snipped out

    break;
    }
    i++;
    }

    It is also worth mentioning that "i" is declared about 200 lines before it is initialized (again) and is not used before or after this block of code.

    Is "i" referenced in the '//some other junk'? I don't see what's wrong with using a counter if you need to know how many you've done (doing something every Nth, or updating a progress bar) while using foreach with the actual iteration - it's easier syntactically since it's automatically filling in the variable with the actual objects you're iterating over.



  • Doesn't settle too well with me. This code assumes that foreach iterates through the collection in the same order as a for loop would, from element 0 to BusinessObjectCollection.Count - 1
    If it does, that's fine. If not, then you've got a nasty bug. I would cut out the foreach, use a for loop instead, and get rid of the uncertainty.



  • @Erick said:

    Doesn't settle too well with me. This code assumes that foreach iterates through the collection in the same order as a for loop would, from element 0 to BusinessObjectCollection.Count - 1
    If it does, that's fine. If not, then you've got a nasty bug. I would cut out the foreach, use a for loop instead, and get rid of the uncertainty.
    You are kidding right?... RIGHT?

    Or you missed the break... yeah that must be it... I hope... 



  • @Daid said:

    @Erick said:

    Doesn't settle too well with me. This code assumes that foreach iterates through the collection in the same order as a for loop would, from element 0 to BusinessObjectCollection.Count - 1
    If it does, that's fine. If not, then you've got a nasty bug. I would cut out the foreach, use a for loop instead, and get rid of the uncertainty.
    You are kidding right?... RIGHT?

    Or you missed the break... yeah that must be it... I hope... 

    Don't be so condescending. He has a perfectly valid point in that there is no guarantee that the enumerator for the collection iterates in the same [i]order[/i] as a for loop would.

    And as for the use of "i", it's never used again. I promise.



  • Well Erick may not be a .net programmer or may have just missed the "break", but Random832's comments are really making me cry.



  • @djork said:

    Don't be so condescending. He has a perfectly valid point in that there is no guarantee that the enumerator for the collection iterates in the same [i]order[/i] as a for loop would.

    I don't think that's what he was taking issue with... 



  • @Random832 said:

    Is "i" referenced in the '//some other junk'? I don't see what's wrong with using a counter if you need to know how many you've done (doing something every Nth, or updating a progress bar) while using foreach with the actual iteration - it's easier syntactically since it's automatically filling in the variable with the actual objects you're iterating over.

    No, "i" is not used anywhere else. And it is, in fact, completely and utterly pointless, as the only time that it [i]is[/i] used is to access the "current" element of the collection which the foreach should give you anyway.



  • I am a .net programmer, and I did not miss the break. I was taking issue with mixing the foreach iterator with the i++ one.
    You either embrace the i iterator and use a for loop, or get rid of i altogether, take advantage of your foreach, and change:

    _instanceVar.BusinessObjectCollection[i].AProperty = something;

    to

    bO.AProperty = something;

    which is actually a much better approach than my previous post.



  • You either embrace the i iterator and use a for loop, or get rid of i altogether, take advantage of your foreach

    And what if you really want to take advantage of the foreach, so you don't have to write something[i] every time you want to do anything (or add an extra declaration, whatever), but there's _one_ thing [among many others that don't] that explicitly needs i as the _quantity_ of times the loop has run [as opposed to as any sort of ordinal index]



  • My point is, the only advantage that

    For i as Integer = 0 to theCollection.Count
        '...do stuff with theCollection(i)
        progessBar.value = i
    Next

    has over 

    Dim i as Integer = 0
    ForEach theObject as Object in theCollection
        '...do stuff with theObject
        i += 1 : progressBar.value = i
    Next

    is some perceived philosophical purity that in reality makes it harder to read, implies order matters when it doesn't, etc. Yes, the code is a WTF, but not solely for mixing a counter with foreach.

    And, if C# is anything like C, For loops are even uglier than they are in VB. 



  • for(int i = 0; i != 5000; i++)

    {

            Collection[i] = 'HI';

    }

     

    Nothing ugly about it.



  • @Random832 said:

    And what if you really want to take advantage of the foreach, so you don't have to write something[i] every time you want to do anything (or add an extra declaration, whatever), but there's one thing [among many others that don't] that explicitly needs i as the quantity of times the loop has run [as opposed to as any sort of ordinal index]



    That's fine, you can do that. You would just never use i to reference the array, as was done in the original code.

    int i = 0;
    foreach (Widget w in WidgetArray)
    {
        if (w.Flag)
        {
            w.DoStuff();
            i++;
        }
    }
    System.Console.WriteLine("I did stuff {0} times!", i)



  • Aw, come on, they just changed for loop that was using the i counter to foreach loop and didn't quite finish the change. Naturally the fact that i was defined 200 lines above played its role too.

    That's why I always try to change types, variable names and what-not when refactoring, so it doesn't compile untill everything changed. Still once in a while I get that "working by accident" code too.



  • @djork said:

    In a 3800 line C# file for a [i]single web control[/i] in our system (all class and instance names changed to protect the profoundly moronic):

    i=0;
    foreach(BusinessObject bO in _instanceVar.BusinessObjectCollection)
    {
    if(theNumberWeAreLookingFor == bO.SomeNumber)
    {
    _instanceVar.BusinessObjectCollection[i].AProperty = something;
    // some other junk snipped out

    break;
    }
    i++;
    }

    It is also worth mentioning that "i" is declared about 200 lines before it is initialized (again) and is not used before or after this block of code.

     

    A somewhat similar pattern is actually necessary in PHP, sans the iterator i, if you actually want to change anything in _instanceVar.BusinessObjectCollection; any changes to b0 itself are actually made to a copy of an element of _instanceVar.BusinessObjectCollection.

     e.g., say you want to set the first instance of the value 6 in an array to 12:

    foreach($array as $i => $value) {

      if($value == 6) {

        $array[$i] = 12;

        break;

      }
     

    }

    If you, instead, attempted to set $value = 12, your array, $array, would remain unchanged.
     



  • @merreborn said:

    A somewhat similar pattern is actually necessary in PHP, sans the iterator i, if you actually want to change anything in _instanceVar.BusinessObjectCollection; any changes to b0 itself are actually made to a copy of an element of _instanceVar.BusinessObjectCollection.

     e.g., say you want to set the first instance of the value 6 in an array to 12:

    foreach($array as $i => $value) {

      if($value == 6) {

        $array[$i] = 12;

        break;

      }
     

    }

    If you, instead, attempted to set $value = 12, your array, $array, would remain unchanged.
     

    According to the PHP online documentation, you can get around that by doing "foreach ($array as $i => &$value)". I don't know if the same problem (and/or solution) exists in other languages such as Perl, though.
     



  • I don't know how much slower/faster this would be but an easy solution to this would be (using a simple example):

    $key = array_search(6, $array);

    $array[$key] = 12;

     
     



  • @Quietust said:

    @merreborn said:
    A somewhat similar pattern is actually necessary in PHP, sans the iterator i, if you actually want to change anything in _instanceVar.BusinessObjectCollection; any changes to b0 itself are actually made to a copy of an element of _instanceVar.BusinessObjectCollection.

    According to the PHP online documentation, you can get around that by doing "foreach ($array as $i => &$value)". I don't know if the same problem (and/or solution) exists in other languages such as Perl, though.
     

    In Perl, the foreach iterator variable is an alias to the relevant cell in the array you are iterating over. This does what it looks like it should: foreach my $i (@a) {$i++}.

    If you do not want this behaviour, you can simply copy the variable or the array, as in foreach my $i (@a) {my $x = $i; $x++} or foreach my $i (@{ [@a] }) {$i++}


Log in to reply