Comment On Break Out

I would argue that the FOR loop is pretty much the first thing you should learn in any language. This coder clearly disagrees.

The comment saves it a little.

34,598th (i.e. last)

Come on. Everybody knows this situation calls for:

i = dtModules.Rows.Count + 1; //used to jump out of the for

It's way more portable and doesn't rely on magistical numbers.

Maybe he was confused as to which number would be just above what would fit in a 2 byte storage (32,768). Of course, that only deals with the seemingly randomness of the number and not the insane ignorance of basic concepts.

Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
2) boxing/unboxing with ToString(): fail
3) comparing string versions of numbers : fail
4) i = 34598 instead of break isn't that horrible, performancewise - just sad.

Bravo, that's the first WTF that's actually made me laugh out loud in a while.

What the heck, at least he wrote a comment that explained why he was doing it.

An Error Occurred
An Error Occurred

frits:

It's way more portable and doesn't rely on magistical numbers.

Why use addition when for loops can be used to solve anything.

    for (int j = 0; j < dtModules.Rows.Count; j++)

    {

        i++;

        if (i >= dtModules.Rows.Count)

            j = 34598;

    }

eeth:

Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
2) boxing/unboxing with ToString(): fail
3) comparing string versions of numbers : fail
4) i = 34598 instead of break isn't that horrible, performancewise - just sad.

1. Looks feasibly like it could be C++, which does not have foreach.
2. Might be incompatible types. As an excuse, this is weak, I know, especially in C++ where we could write an operator ==() that compares the two types.
3. They are IDs, not numbers. Why assume that IDs are numbers?
4. It is worse than sad. It will one day cause a weirdrandomfailure.

Steve the Cynic:

eeth:

Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
2) boxing/unboxing with ToString(): fail
3) comparing string versions of numbers : fail
4) i = 34598 instead of break isn't that horrible, performancewise - just sad.

1. Looks feasibly like it could be C++, which does not have foreach.
2. Might be incompatible types. As an excuse, this is weak, I know, especially in C++ where we could write an operator ==() that compares the two types.
3. They are IDs, not numbers. Why assume that IDs are numbers?
4. It is worse than sad. It will one day cause a weirdrandomfailure.

If it was C++, the removal method wouldn't be as succinct. The preferred idiom would be some crap like:

dtModules.Rows.back_remover(dtModules.Rows.end_iter(), dtModules.Rows.end_iter() - 1);

And this code would of course be hidden in a functor class that requires half a dozen templated parameters to initialise it.

I fscking hate C++. In fact I hate it so much I've started twitching uncontrollably. Can someone pull the screens round me and find the nurse?

[quote user="eeth"]Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
...

But, using a foreach over a collection, then removing an item from that collection during the loop, is also fail.

#define END_FOR_LOOP 34598

for (int i = 0; i < dtModules.Rows.Count; i++)
{
if (dtModules.Rows[i]["id"].ToString() == ID.ToString())
{
dtModules.Rows.RemoveAt(i);
i = END_FOR_LOOP; //used to jump out of the for
}
}

There, fixed.

eeth:

Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
2) boxing/unboxing with ToString(): fail
3) comparing string versions of numbers : fail
4) i = 34598 instead of break isn't that horrible, performancewise - just sad.

WTF is wrong with a for loop?

captcha: nulla -- Valley girl for Null (i.e. "That variable was so nulla, it like gave me an Object reference not set to an object error.").

1. Looks feasibly like it could be C++, which does not have foreach.
2. Might be incompatible types. As an excuse, this is weak, I know, especially in C++ where we could write an operator ==() that compares the two types.
3. They are IDs, not numbers. Why assume that IDs are numbers?
4. It is worse than sad. It will one day cause a weirdrandomfailure.

1: If my memory serves, C++ has no ToString()
2: Conceded.
3: If they're not numbers then they're already string and don't need the ToString().
4: Hence the initial invocation of the Almighty!

probably he first tried:
i = Integer.MAX_VALUE;

but found out that this did not work as expected. Naturally
i = 34598
was the next choice!

highphilosopher:

<snip>
WTF is wrong with a for loop?
</snip>

Nothing's wrong with a for loop, but a foreach is generally more efficient.

java.lang.Chris;:

If it was C++, the removal method wouldn't be as succinct. The preferred idiom would be some crap like:

dtModules.Rows.back_remover(dtModules.Rows.end_iter(), dtModules.Rows.end_iter() - 1);

And this code would of course be hidden in a functor class that requires half a dozen templated parameters to initialise it.

I fscking hate C++. In fact I hate it so much I've started twitching uncontrollably. Can someone pull the screens round me and find the nurse?

Whilst I share your hate of template rubbish like that, which seems to have infected a lot of the standard ideas for C++, I do feel like pointing out that it's not the fault of the language. It's the fault of the libraries, including the standard library.

Qt's list container has a lovely removal function - http://doc.trolltech.com/4.6/qlist.html#removeAt (I can't link it without being marked as spam). It's short, obvious and doesn't vomit template code all over your face and torso.

Anyway, the article code is probably C#.

What happens if there are *two* objects with the corresponding ID? D:

eeth:

1) not using foreach : fail
2) boxing/unboxing with ToString(): fail
3) comparing string versions of numbers : fail
4) i = 34598 instead of break isn't that horrible, performancewise - just sad.

I just cannot agree with item 1. I've go no idea what language this is (C#?), but foreach does precisely the same. Only this code also makes it clear that it is the first occurrence of ID that will be deleted. With foreach, it stays more implicit IMO. And breaking out of a foreach sort of contradicts the term itself...

eeth:

1: If my memory serves, C++ has no ToString()

You can make a ToString function in C++.

eeth:

3: If they're not numbers then they're already string and don't need the ToString().

Top computer scientists across the world have discussed the possibility that there may be other data types than just ints and strings.

Or, you know:

DELETE FROM Modules WHERE id = i

I'd go for

try
{
for (int i = 0; i < dtModules.Rows.Count; i++)
{
if (dtModules.Rows[i]["id"].ToString() == ID.ToString())
{
dtModules.Rows.RemoveAt(i);
throw new BreakException();
}
}
}
catch {}

eViLegion:

eeth:

Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
...

But, using a foreach over a collection, then removing an item from that collection during the loop, is also fail.

That only matters if you continue in the foreach, so removing and then break works.

Absolutely brilliant.

chodger:

You can make a ToString function in C++.

Top computer scientists across the world have discussed the possibility that there may be other data types than just ints and strings.

Whomever wrote this for loop didn't have the skills to write a ToString() in C++.

There are absolutely more data types than integers and strings. That's not what I said, though. And fyi, even strings reduce to numeric representations.

I disagree with number 1 for a different reason. If you use a foreach, you generate an enumerator. While iterating on the enumerator you cannot modify the original collection. At least you can't in C#, it generates a runtime exception.

TRWTF is the i=34598, the string comparison may be fail, but we don't actually know what the data type of the second dimension of the dictionary is, so it could actually be a reference type, in which case, the code maybe correct in doing a ToString() in order to compare it to a number.

eeth:

Almighty save us all if dtModules ever contains 34598 rows.

1) not using foreach : fail
2) boxing/unboxing with ToString(): fail
3) comparing string versions of numbers : fail
4) i = 34598 instead of break isn't that horrible, performancewise - just sad.

1)If the source is Java, foreach did not exists until 1.6
2)Even more puzzling....is it really checking to see if the String references are the same?
Again, assuming Java, shouldn't it be

if (dtModules.Rows[i]["id"].ToString().equals(ID.ToString()))

if Strings are really required for the comparison...
3)Which makes point 2 even weirder...why bother with toString instead just doing the quick arithmetic comparison?
4)I feel better about my day now...but not by much.

3rd attempt...

The programmer needs the index of the item in order to remove it, which is not possible with foreach.

for (int i = dtModules.Rows.Count-1; i && dtModules.Rows[i]["id"].ToString() != ID.ToString() ; i--);
if (dtModules.Rows[i]["id"].ToString() == ID.ToString()) dtModules.Rows.RemoveAt(i);

I know, is a little weird.

In C#, all strings with the same "contents" refer to the same object. C# strings have an .Equals() method too, by the way.

ClutchDude:

1)If the source is Java, foreach did not exists until 1.6

Sorry...that should be 1.5

TRWTF is that none of you geniuses realize that a break statement is just a goto in disguise. Thank dog the original coder was able to avoid that evil...

Slicerwizard:

TRWTF is that none of you geniuses realize that a break statement is just a goto in disguise. Thank dog the original coder was able to avoid that evil...

So is a for-loop

Phanton_13:

for (int i = dtModules.Rows.Count-1; i && dtModules.Rows[i]["id"].ToString() != ID.ToString() ; i--);
if (dtModules.Rows[i]["id"].ToString() == ID.ToString()) dtModules.Rows.RemoveAt(i);

I know, is a little weird.

1. That does the opposite (removes the last occurrence of ID rather than the first) which may not be an issue.
2. i is referenced out of context after the for loop.

Slicerwizard:

TRWTF is that none of you geniuses realize that a break statement is just a goto in disguise. Thank dog the original coder was able to avoid that evil...

Pfff. ON ERROR RESUME NEXT

Corey:

Or, you know:

DELETE FROM Modules WHERE id = i

Generally, yes. Only that the code did not appear to actually change the database. It only discards a row locally. For all we know, there is no database, only some top notch coder feeling all important by using Row as a member name :-)

Corey:

Or, you know:

DELETE FROM Modules WHERE id = i

LIMIT 1

eeth:

highphilosopher:

<snip>
WTF is wrong with a for loop?
</snip>

Nothing's wrong with a for loop, but a foreach is generally more efficient.

Easier to read, but if you'll check it out you'll find that a foreach usually takes longer to execute than a for loop. Although picking one over the other is such a micro-optimization that's its laughable.

Amateurs! OBVIOUSLY you do it best by:
* Serializing the module list into XML.
* Splitting the XML into a String array of lines
* Searching the lines with a for-case construct until a Regex finds the id you're looking for
* Then Recombining all lines except the one you found to have the right ID using another for loop.
* Deserializing the result back into the object
* And finally throwing a new ExitLoopException which you swallow with a empty catch-all.

eeth:

1. Looks feasibly like it could be C++, which does not have foreach.
2. Might be incompatible types. As an excuse, this is weak, I know, especially in C++ where we could write an operator ==() that compares the two types.
3. They are IDs, not numbers. Why assume that IDs are numbers?
4. It is worse than sad. It will one day cause a weirdrandomfailure.

1: If my memory serves, C++ has no ToString()
2: Conceded.
3: If they're not numbers then they're already string and don't need the ToString().
4: Hence the initial invocation of the Almighty!

1: It does after you implement it.
3: It could be an ID class that stores the ID differently.

This was probably on an embedded system, where the programmer couldn't rely on having a filesystem.

Kempeth:

Amateurs! OBVIOUSLY you do it best by:
* Serializing the module list into XML.
* Splitting the XML into a String array of lines
* Searching the lines with a for-case construct until a Regex finds the id you're looking for
* Then Recombining all lines except the one you found to have the right ID using another for loop.
* Deserializing the result back into the object
* And finally throwing a new ExitLoopException which you swallow with a empty catch-all.

You are so hired!

Surely this can be handled with LINQ.

highphilosopher:

eeth:

highphilosopher:

<snip>
WTF is wrong with a for loop?
</snip>

Nothing's wrong with a for loop, but a foreach is generally more efficient.

Easier to read, but if you'll check it out you'll find that a foreach usually takes longer to execute than a for loop. Although picking one over the other is such a micro-optimization that's its laughable.

It depends on the language and how they're implemented. Generally speaking, I use a foreach when dealing with hashes and fors when dealing with arrays.

I think PHP's manual used to recommend using for loops since foreach was a little bit slower for large arrays. I can't find proof of this, but that's what memory tells me.

eeth:

1) not using foreach : fail

You can't use a foreach statement over the Rows collection because the collection gets changed in the loop (item removed). You will get a compilation error if you attempt to add or remove an item from a collection that you are iterating over with a foreach statement. Using a for loop is not incorrect here, it is the only sensible option.

I somewhat sympathize with the choice to assign the loop index a "too high" value rather than using 'break'. The 'break' statement can create a trap for a maintainer. If he later adds a loop, the 'break' will break out of his inner loop rather than the outer loop, as intended.

Consider, hypothetically, if each row is later changed to contain an array of values, a match on any one of which would select that row. Someone quickly modifying all instances of accesses to the row number to iterate through all values in each row might create a subtle bug where the break doesn't prevent processing of subsequent rows. (Instead it would only break out of the new inner loop and stop checking other numbers for the current row.)

That said, alternative solutions aren't so pretty either. In some cases, 'goto' really is best, but obviously that is pretty ugly. Wrapping the loop into a function that can 'return' is sometimes suitable, but also sometimes completely inappropriate.

I sometimes wish C had a way to name a scope and 'break' or 'continue' a named scope.

Anonymous:

eeth:

1) not using foreach : fail

You can't use a foreach statement over the Rows collection because the collection gets changed in the loop (item removed). You will get a compilation error if you attempt to add or remove an item from a collection that you are iterating over with a foreach statement. Using a for loop is not incorrect here, it is the only sensible option.

Sorry, it's not a compilation error but a runtime error. Either way, it won't work.

How would you react if the code was like this?

int i = 0;

while((dtModules.Rows[i]["id"].ToString() != ID.ToString()) && (i < dtModules.Rows.Count))
{
++i;
}

dtModules.Rows.RemoveAt(i);

No for or foreach! Crazy.

If this was recent c# you wouldn't need a for or foreach (with linq, etc.), but I guess the dev might not have realised that.

Real coders use goto