The Daily WTF: Curious Perversions in Information Technology

2010-01-27 Reply Admin

NeoMojo:
How would you react if the code was like this? <snipped>

I would fire you.

2010-01-27 Reply Admin

NeoMojo:
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);

I would react badly as it tried to (a) query, and (b) remove a row out of bounds every time it didn't get an ID match.

2010-01-27 Reply Admin

NeoMojo:
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.

Damnit that won't work. If the row is never found, it removes one regardless.

int i = 0; bool found = true;

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

if(found) { dtModules.Rows.RemoveAt(i); }

should work better.

Not that I'm a perfectionist at the expense of a joke or anything...

frits · 2010-01-27 Reply Admin

NeoMojo:
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.

Don't forget to trap index out of range. And LINQ is only viable if you plan on targeting XP and newer OS's.

2010-01-27 Reply Admin

NeoMojo:
Damnit that won't work. If the row is never found, it removes one regardless.

Both your attempts query out of bounds in the while loop condition.

bjolling · 2010-01-27 Reply Admin

frits:
Surely this can be handled with LINQ.

Lemme givit a try:

dtModules.Rows.Remove(dtModules.Rows.First(e => e.ToString() == ID.ToString()));

2010-01-27 Reply Admin

Anonymous:
NeoMojo:
How would you react if the code was like this? <snipped>
I would fire you.

You can't fire me, as I'm your bosses son, and your boss has rose tinted spectacles when it comes to me.

What do you do now?

2010-01-27 Reply Admin

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.

No no no no no.

Once you have the module list as XML, you should just feed that to an XSLT processor in order to have that find the right line and then write another XML file with the data you need. This makes that whole step both encapsulated and reusable!

2010-01-27 Reply Admin

blaz:
NeoMojo:
Damnit that won't work. If the row is never found, it removes one regardless.

Both your attempts query out of bounds in the while loop condition.

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

I think.

2010-01-27 Reply Admin

That's the one.

Now do it without any keywords.

2010-01-27 Reply Admin

joelkatz:
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.

Breaking out of a named scope is better than using a goto, in that it doesn't use the 'goto' keyword.

Dogma makes me angry. There are situations where goto is the appropriate tool for the job. The fact that it doesn't place restrictions that make it difficult to use incorrectly does not make it bad.

TRWTF is creating things like named scopes for no apparent purpose other than to avoid 'goto'.

2010-01-27 Reply Admin

...bet the programmer was Indian.

:-O

2010-01-27 Reply Admin

NeoMojo:

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

I think.

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

Howdya like dem tomatoes? ;)

2010-01-27 Reply Admin

NeoMojo:
Anonymous:
NeoMojo:
How would you react if the code was like this? <snipped>
I would fire you.

You can't fire me, as I'm your bosses son, and your boss has rose tinted spectacles when it comes to me.

What do you do now?

Damn you NeoMojo! I guess I'm just going to have to keep my mouth shut and complain about you quietly behind your back. You win this round!!

2010-01-27 Reply Admin

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.

Needs more cowbell.

pjt33 · 2010-01-27 Reply Admin

To all those people who say that you get an exception if you remove an element from a collection while iterating over it: when do you get the exception? In Java it would be when you ask for the next element, having done the removal, so if you do the removal and immediately break out of the loop you're fine.

2010-01-27 Reply Admin

NeoMojo:
Anonymous:
NeoMojo:
How would you react if the code was like this? <snipped>
I would fire you.

You can't fire me, as I'm your bosses son, and your boss has rose tinted spectacles when it comes to me.

What do you do now?

Duh. I fire me.

An Error Occurred

2010-01-27 Reply Admin

Maybe the developer was coding for an embedded environment ... one for which the full set of grammar had not been implemented in its compiler?

Coyne · 2010-01-27 Reply Admin

DT:
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

Actually, I think he was being sarcastic. But his point wasn't branching: It was structured programming rules. The "break", "return", and "throw" constructs can all be used to violate structured programming rules, strictly speaking.

2010-01-27 Reply Admin

I raise: ... if (dtModules.Rows[i]["id"].ToString() == ID.ToString()) { dtModules.Rows.RemoveAt(i); //cause overflow to break out of loop i = 2147483647 + 1;
} ...

SCSimmons · 2010-01-27 Reply Admin

NeoMojo:
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.

I'd been about to respond to the poster that thought it was c# that it couldn't be, because there were so much better ways to do this in c# that nobody could possibly have written a travesty like that.

Then I remembered where I was, and decided to just go cry quietly in the corner instead.

2010-01-27 Reply Admin

The poor original writer MAY have decided to use that weird exit strategy if he'd read that book about how AT&T lost $110 million because of a misplaced "break" in their phone-system code. You remember the big long-distance blackout of January 1990?

Not that his way is measurably better.

2010-01-27 Reply Admin

daqq:
Real coders use goto

Real coders use JZ/JNZ.

drachenstern · 2010-01-27 Reply Admin

JAn:
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!

Nonsense, he forgot to use a wooden table. You can mail me your offer-of-employment to my address on file in your system. I faxed the polaroid of the extruded plastic numbers to your blackberry.

WIN :D

2010-01-27 Reply Admin

Apart from the magic number there is one other WTF here and that is using linear search. That is, unless the collection is going to be small enough that it won't matter. Certainly if it is anywhere near 34000 elements it is clearly wrong.

Why are so many people commenting on nonsense like using for vs foreach vs std::find or whatever and not noticing the bigger picture?

toth · 2010-01-27 Reply Admin

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

not using foreach : fail ...

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

Unless you are

break

ing out right afterwards.

2010-01-27 Reply Admin

Another possible inefficiency (that doesn't seem to have been noticed).

Although you might have to call ToString(), you only need to call it once on ID - that never changes, so unless it's pretty much a no-op (i.e. a Get function) call it before the loop.

drachenstern · 2010-01-27 Reply Admin

partisan:
daqq:
Real coders use goto

Real coders use JZ/JNZ.

obXKCD (on a forum which despises XKCD - yes, I know the rules, but this being the front page article, at least we can get it over with)

http://xkcd.com/378/

campkev · 2010-01-27 Reply Admin

Anonymous:
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.

I thought the same thing at first, but tested it to make sure and found that's not the case. The exception won't get called until the next iteration, and since you are breaking after the removal, it would execute ok. HOWEVER, I would still consider using a foreach bad form here as you are setting yourself up for failure later when you decide you don't want to just remove the first, but all instances and remove the break (or the i=somerandomnumber, as the case may be).

eeth · 2010-01-27 Reply Admin

campkev:
I thought the same thing at first, but tested it to make sure and found that's not the case. The exception won't get called until the next iteration, and since you are breaking after the removal, it would execute ok. HOWEVER, I would still consider using a foreach bad form here as you are setting yourself up for failure later when you decide you don't want to just remove the first, but all instances and remove the break (or the i=somerandomnumber, as the case may be).

Now that's a good point. Maintainability ftw.

2010-01-27 Reply Admin

Add me to the crowd that thinks for is better than foreach here. Even if the foreach approach doesn't cause an error it's implying the wrong thing. I would never code a foreach that terminated in the middle (other than a major error bailout.) Foreach implies that you are covering the whole group!

RogerC · 2010-01-27 Reply Admin

Maybe he thought that using magic numbers made hm a coding wizard.

2010-01-27 Reply Admin

pjt33:
To all those people who say that you get an exception if you remove an element from a collection while iterating over it: when do you get the exception? In Java it would be when you ask for the next element, having done the removal, so if you do the removal and immediately break out of the loop you're fine.

The exception will be thrown when you make the call that modifies the collection. For example, if you are iterating over a List and do something like this:

foreach (string str in myList)
{
  Console.WriteLine("Item is " + str);
  myList.Remove(str);
}

...the exception will be thrown on the call to List.Remove(). So it is not possible to modify the collection then immediately break out of the loop. You have to use a different type of loop.

2010-01-27 Reply Admin

Thinking a foreach is better here than a for is a major WTF, and I dare say, even a bigger WTF than the article. There is absolutely NO reason to pick a foreach here.

Also, I thought of a more elegant solution:

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

2010-01-27 Reply Admin

1: C++ can always have ToString(). Not a problem to write one... 3: It may be more complex class, so ToString() methot is acceptable. But of course ir should be opertor==().

2010-01-27 Reply Admin

frits:
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.

You'll have serious problems if dtModules.Rows.Count is MAX_INT though (where MAX_INT is the maximum possible value to fit into that sized variable).

2010-01-27 Reply Admin

pjt33:
To all those people who say that you get an exception if you remove an element from a collection while iterating over it: when do you get the exception? In Java it would be when you ask for the next element, having done the removal, so if you do the removal and immediately break out of the loop you're fine.

Sorry guys, disregard what I just said, I was wrong. Campkev is correct here:

campkev:
I thought the same thing at first, but tested it to make sure and found that's not the case. The exception won't get called until the next iteration, and since you are breaking after the removal, it would execute ok. HOWEVER, I would still consider using a foreach bad form here as you are setting yourself up for failure later when you decide you don't want to just remove the first, but all instances and remove the break (or the i=somerandomnumber, as the case may be).

2010-01-27 Reply Admin

I'd have done it all in sql. That's what it is there for.

2010-01-27 Reply Admin

top...men...

captcha: vulputate to amputate one's vulva

2010-01-27 Reply Admin

Coyne:
DT:
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

Actually, I think he was being sarcastic. But his point wasn't branching: It was structured programming rules. The "break", "return", and "throw" constructs can all be used to violate structured programming rules, strictly speaking.

...as could the modification of control variables, as the case shows. There are people that avoid 'break', 'continue', and 'return' at any cost but achieve the same effect by modifying the state of program control variables. At the end, its the same (e.g. premature loop break) only worse, since it is hidden. My bet is that the guy that wrote the code wanted to avoid using break.

2010-01-27 Reply Admin

WIN!!! lol nice one.

2010-01-27 Reply Admin

Not sure on the specific language but foreach would normally give you an element reference and not something you can subsequently remove.

If you were writing this in C++ there is find_if which is probably the algorithm you would use here if you have to use linear search at all.You can then do erase() on the iterator if it is not end().

Using find_if might look messy but one is highly unlikely to fall into the trap of calling ID.ToString() on every iteration by using it as you would pass that in as a parameter to something (possibly some horrible bind, yes).

2010-01-27 Reply Admin

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

not using foreach : fail ...

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

LOL WIN!!! nice one... sorry for the dbl post... I thought replying would make it easy to see that my post was related to the above quote... instead it leaves a small unnoticable note that says that this message is a reply to [link]

eeth · 2010-01-27 Reply Admin

person1:
LOL WIN!!! nice one... sorry for the dbl post... I thought replying would make it easy to see that my post was related to the above quote... instead it leaves a small unnoticable note that says that this message is a reply to [link]

Sorry, incorrect. You absolutely can remove an item from a collection while iterating it. It might cause a runtime index exception, but it will not if the loop is exited.

2010-01-27 Reply Admin

eeth:
person1:
LOL WIN!!! nice one... sorry for the dbl post... I thought replying would make it easy to see that my post was related to the above quote... instead it leaves a small unnoticable note that says that this message is a reply to [link]

Sorry, incorrect. You absolutely can remove an item from a collection while iterating it. It might cause a runtime index exception, but it will not if the loop is exited.

It doesn't cause any type of index exception, it causes an InvalidOperationException as the state of the collection is invalid for the given operation (which will be MoveNext on the iterator).

2010-01-27 Reply Admin

If you want to remove multiple items in C++ from a collection, STL provides you std::remove_if. However that doesn't actually remove the items. You need to put collection.erase before it and then collection.end() as the second parameter.

Does this look mad? Yes - but you should learn to use your collections correctly.

Use multimap or map, Then a plain erase(value) will do, and doesn't matter if value doesn't exist. (Can also use hash_map of course, albeit that it isn't standard). You can use map from key to a collection instead of using multimap too (eg map(key,std::list<item>)
If you have multiple indexes on a collection then use std::list as the collection type. Best collection if you are going to remove items from the middle and not invalidate any iterators. Have you indexes point to iterators. Of course if you delete items you must fix all your indexes but once you find the item you can probably find it in all your indexes easily enough. If it's a huge collection and you're removing only a few items, and you want to save on time rather than space this is the way to do it.

2010-01-27 Reply Admin

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

This couldn't be further from the truth. While it's possible for string interning to take place such that multiple strings with the same contents reference the same string object in memory, it's not always the case.

Example App:

using System;

public class MyClass { public static void Main() { string first = "MyString"; string second = "My"; second += "String";

	Console.WriteLine("Reference Equals: " + ReferenceEquals(first, second));
	Console.WriteLine("Content Equals: " + first.Equals(second));
}

}

2010-01-27 Reply Admin

DT:
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

Look, any type of branching violates the "no-go-to" rule, and, it slows down the processor. For maximum efficiency refactor all your code to run in a straight line. After all only one path is going to be executed so why write all those other paths? It just makes testing harder.

frits · 2010-01-27 Reply Admin

EatenByAGrue:
frits:
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.

You'll have serious problems if dtModules.Rows.Count is MAX_INT though (where MAX_INT is the maximum possible value to fit into that sized variable).

Time to get your sarcasm detector recalibrated.

2010-01-27 Reply Admin

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

This couldn't be further from the truth. While it's possible for string interning to take place such that multiple strings with the same contents reference the same string object in memory, it's not always the case.

Example App:

using System;

public class MyClass { public static void Main() { string first = "MyString"; string second = "My"; second += "String";
	Console.WriteLine("Reference Equals: " + ReferenceEquals(first, second));
	Console.WriteLine("Content Equals: " + first.Equals(second));
}
}

Brian is correct. Just because strings are immutable in C# it does not mean that all like strings are the same object in memory. Look up string.Intern() and string.IsInterned() if you want more information on this.

Break Out

Leave a comment on “Break Out”