Seneral / Node_Editor_Framework

A flexible and modular Node Editor Framework for creating node based displays and editors in Unity
https://nodeeditor.seneral.dev
MIT License
2.01k stars 414 forks source link

Use foreach instead of for, for code readability. #52

Closed groud closed 8 years ago

groud commented 8 years ago

This greatly improves the code readability, at it does not need a "counter" variable.

pmhpereira commented 8 years ago

Probably because of the better performance.

Shouldn't be a issue, because all the Lists are pretty small.

Seneral commented 8 years ago

Huh, I somewhere read that when you reference the iterated variable more than twice in the loop, you should use foreach, else it's faster to use for... Which is correct now?

Seneral commented 8 years ago

http://www.dotnetperls.com/for-foreach Maybe you can adjust this to when a variable was only acessed once, leave it as for, and when it's acessed multiple times (or sometimes even the cached value), change it to forach? That's best for performace. But as pmhpereira said, doesn't really matter anyway. I'm simply used to use for loops instead of foreach most of the time;)

groud commented 8 years ago

In fact the performance issue truely depends on the structure you are iterating on. But however, at this scale we are not talking at all about performance.

I think that the code would greatly benefit from such modification, not because it would perform better, but because that is FAR more readable. I think at this point it is more important than performance. :)

Seneral commented 8 years ago

^^ Can you fix these three occurances to a for loop again? I think that is the only way:/

groud commented 8 years ago

I'll have a look this evening. :)

By the way, concerning about the performance, according to this and this, when a foreach loop is applied to an array, it is compilated into a normal indexing loop (well a good compilator should...) . Unless they are wrong, both solutions must be equivalent in terms of performance. :)

Baste-RainGames commented 8 years ago

Unity's old mono compiler has a bug where foreach loops generates (40 bytes of) garbage on anything that's not a normal array (usually a List, like in the conditions list here).

This has caused a huge amount of confusion on the forums about what's good practice and what's not. Generally, I'd say that that this is not an issue, and the readability wins out. This is a plugin though, and it's nice to be able to use plugins like a black box without having to worry about their memory usage.

Up to you guys, I just figured I'd chip in the facts.

Seneral commented 8 years ago

Thanks for the clarification:) I'd be ok with the for loops besides the above mentioned three exceptions where a for loop is needed. Performance isn't really something to look at too closely at the moment, there would be other parts to first look at for optimizations:) Then I would merge if everyone is oke with that.

groud commented 8 years ago

Done. :)

Seneral commented 8 years ago

Anyway, can be added at a later date;)

Seneral commented 8 years ago

Seems like one of the commits throws an error where you replaced the for with a foreach loop, but the cnt is still referenced in the loop. Reverting back to for loop, this is necessary.

Edit: Alot of errors. Did not test this out previously but a comment on the thread complained about errors. And there are alot of them:( Please test the changes out before you commit;)