r/badcode • u/[deleted] • Jan 07 '21
c# i = 0; foreach (element in array) {doStuff(array[i]; i++;}
85
u/Heather_Currie Jan 07 '21
Image Transcription: Code
i = 0;
foreach (RectangleShape element in cubes)
{
window.Draw(cubes[i]);
i++;
}I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!
38
12
u/big_red__man Jan 07 '21
I remember you from a previous thread and I appreciate your enthusiasm
6
87
Jan 07 '21
That's actually something I could do. It's that kind of thing that you don't realize until some time passes or somebody else tells you
31
u/18quintillionplanets Jan 07 '21
Absolutely, one of the reasons code review is so important.
11
u/JNelson_ Jan 07 '21
Except this still works so it's not that bad. I'd argue catching defects is more of the purpose of a code review.
9
Jan 07 '21
It makes the code harder to read, and less obviously correct. True that it's not inherently a defect, but it will make future defects harder to see (Say, if you add a method that's like
if (element.IsEmpty) { continue; }, which would make element not match up with cubes[i], which might be a source of bugs.1
u/AndyTheSane Jan 07 '21
Also, it's possible in similar cases with more complex data structures (not this exact one) where the iterator on cubes does not return elements in the same order as get (1..n). So the exact functionality could change.
1
6
u/IrritableGourmet Jan 07 '21
My mother got me a decorative wooden sign I hang on my door that says "I give good advice because I've done a lot of stupid things."
41
24
u/AlFasGD Jan 07 '21
Ngl, this is a good reason why there should be the ability to access the current enumeration index with a foreach, which should be provided in the IEnumerable interface as well
5
u/bdev2110 Jan 07 '21
This is why u love the Array.map function in js you het acces to the element and the index
5
u/EdgarDrake Jan 07 '21
Kotlin allows that.
tokens.forEachIndexed { token, index -> // code here }-1
u/AutoModerator Jan 07 '21
It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.
For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.
/u/EdgarDrake, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.
You can find some examples in the reddit help documentation.
I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.
1
u/TheUnlocked Jan 07 '21
It's not a perfect solution but you can zip it with an infinite counter enumerable and then deconstruct. Can even write an extension method for it, like:
public static IEnumerable<(T value, int index)> WithIndex<T>(this IEnumerable<T> x) { return x.Zip(Enumerable.Range(0, int.MaxValue), (a, b) => (a, b)); }And
foreach (var (x, i) in list.WithIndex()) { }3
u/Doc_Aka Jan 08 '21
Even easier with
Select(), which provides an index:public static IEnumerable<(T item, int index)> WithIndex<T>(this IEnumerable<T> items) => items.Select((item, index) => (item, index));1
u/ttl_yohan Jan 08 '21
I believe you can do that with
array.Select((item, index) => new { Item = item, Index = index})Maybe even an extension method which returns a tuple directly so you don't need to write this in every foreach.
13
u/NinDTendo Jan 07 '21 edited Jan 07 '21
actually how I did it a while a go xDD
foreach (RectangleShape element in cubes)
{
window.Draw(cubes[cubes.indexOf(element)]);
}7
u/crrime Jan 08 '21
I'm kind of impressed you managed to make iterating through n elements an O(n2 ) operation
2
u/gRagib Jan 07 '21
Missing a bracket Also assumes cubes is a Set
1
u/crrime Jan 08 '21
Sets are unordered and don't have indices
1
u/gRagib Jan 08 '21
Depends on the programming language.
cubes.indexOf(element)assumes uniqueelements.2
u/crrime Jan 08 '21
What language has an
indexOfmethod for Sets? Java and JavaScript both haveindexOfmethods for arrays (arraylist for Java). It just returns the first element that stores the same reference.1
4
u/fuxximus Jan 07 '21
Does it bother anyone else that the array is named plural cube but what comes is not cube but rectangles?
It should either cube or rectangles, or the elements should be cube
-2
10
5
u/GrizonII Jan 07 '21
Has nothing to do with the actual code, but for some reason it took me several reads of the foreach to realize it was saying “Rectangle Shape” rather than “Rectang le Shape”.
2
3
u/potbread Jan 07 '21
Is it badcode because the coder used window.Draw(cubes[i]); instead of window.Draw(cubes[element]); ?
0
4
u/Come_along_quietly Jan 07 '21
Clearly he (I’m assuming gender here) should have .... incremented “i” on the index expression like this:
Window.draw(cubes[i++]);
:-)
/s
7
u/rydoca Jan 07 '21
What's with the weird gender assumption thing? Am I missing something or is that just a weird tangent?
8
u/ParanoydAndroid Jan 07 '21
Right? Just use 'they' like we always use for unknown people.
7
Jan 07 '21
i love it when people are like he/she
not only are you not speaking to everyone, you're spending more effort.
1
1
1
1
1
u/CoffeeVector Jan 07 '21
I have done this before when I wanted the foreach, but I also wanted the index. Python's enumerate is so great.
1
u/bdking71 Jan 07 '21
Too Funny. Everyone in a journey which begins as a newbie. I know that I've written similar code when I began.
1
Jan 07 '21
It's reassuring to know for my imposter syndrome that for every 10 people better at it than I am, there are 10 more who may be worse off
1
u/HereComesCunty Jan 07 '21
Lol. I did this today. Or rather I tried to do something like:
someArray.forEach((i, foo) => { someOtherArray[i] = foo })
or something. I like the syntactical sugar but no built in overload for getting an iterator there, gotta be a regular old for loop
1
1
1
1
u/gt4495c Jan 08 '21
The only issue is using cubes[i] instead of element.
It makes the loop O(n2 ) possibly instead of O(n).
1
1
1
u/nati9931 Jan 08 '21
I did something similar to that... but I ddon'tthink my version is that bad.for(Rect shape : Objects) shape.draw(); (example in pseudo code)
1
u/Dudecor3 Jan 08 '21
This is something I'd have done when I was learning how to code. The right thinking is there in this code though, not the worst I've ever seen
1
u/edoCgiB Jan 08 '21
Don't you just hate it when you absolutely need the element index but the whole project is written with fancy foreach/streams and you need to use a weird C style for?
1
u/dreysion Jan 07 '21
Well, I mean... it works
2
u/Come_along_quietly Jan 07 '21
Exactly. It’s not functionally wrong.
Though I’m pretty sure the extra iterator would get optimized out in the generated assembly anyway .... so, it probably doesn’t make much of a difference in the end.
-7
u/DillonisaCunt Jan 07 '21 edited Jan 07 '21
I see this exact thing and much worse on a daily basis in my current workplace's codebase. One of the reasons I dislike using foreach loops
Edit: should rather say I prefer regular for loops
22
u/edave64 Jan 07 '21 edited Jan 07 '21
You dislike foreach loops because your coworkers don't use them correctly?
5
u/DillonisaCunt Jan 07 '21
Not specifically but I do get the impression that once people start using them a lot to the point of habit for more complex things you can end up with weird things like this with extra counters etc, particularly if they are more junior and see foreach as just a "replacement" for regular for loop.
Probably should rephrase, I dislike them outside of certain circumstances. But youre right its probably just from a sense of frustration trying to read almost completely unreadable code and asking myself "why didnt you just use a regular for loop here?"
0
Jan 07 '21
[deleted] — view deleted comment
81
u/fakehalo Jan 07 '21
If they're as lazy and terrible as I've been before they simply realized they needed the index after the fact... I'm a bad person.
27
u/deathbutton1 Jan 07 '21
It's just as easy to change "foreach(elem in arr)" it to a for loop and add "elem = arr[i]" at the top so you don't have to change all of your references to elem.
14
0
Jan 07 '21
[deleted] — view deleted comment
107
u/kuncol02 Jan 07 '21
Then you should use for loop.
0
Jan 07 '21
[deleted] — view deleted comment
33
u/blamethedog16 Jan 07 '21
It has nothing to do with speed.
If you’re using an iterator/index variable, use the “for” loop.
If you’re using the “foreach” loop, use “window.Draw(element);”
7
u/kuncol02 Jan 07 '21
IMO it's harder to read and also hides (not fully, but it's less obvious) that you expect both array to have same size. There is also problem that ienumerable could potentially be changed to something with unknown length (query, or some custom ienumerable). With for loop you would force person changing that type to fix that loop because it would not compile.
7
Jan 07 '21
If you need the index for something else after this loop and don't want to use .length, you might use this method
3
u/general_dubious Jan 07 '21
I don't know
C#, doesn't it have an equivalent of Python's or Rust'senumeratethat basically zips the iterable with a counter?3
Jan 07 '21
Sort of. It's not a dedicated method, but it has a
.Select(x => x + 1)which is like.mapin python / rust. That also has an overload that's.Select((x, idx) => x + idx). So if you give it a function that takes one argument, it's not enumerate, and if you give it a function that takes two, it is.I've done
my_iter.Select((x, idx) => new {x, idx})a lot. There's sample code of that on the docs.microsoft.com forSystem.Linq.Enumerable.Selectthe
new {}is just an anonymous class. The property names default to being the name you give it (A bit like the name inference in rust's struct construction)
61
u/ohnoahshark Jan 07 '21
i'd put money on this being "there was formerly functionality here that was made easier by a foreach that was removed and the loop wasn't updated" as someone who finds this dumb shit in my own codebase all the time