[CLOSED] Fix #1098: Some code uses $.each, some uses Array.prototype.forEach

[core-ai-bot]

Issue by TomMalbran
Sunday Mar 10, 2013 at 07:54 GMT
Originally opened as adobe/brackets#3087

---

This fixes #1098 by replacing every $.each in the core code with either Array.forEach for arrays or CollectionUtils.forEach for objects. The remaining $.each are only in jQuery.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/3087/commits

[core-ai-bot]

Comment by pthiess
Tuesday Mar 12, 2013 at 17:26 GMT

---

Community pull request - open to anyone to grab.

[core-ai-bot]

Comment by WebsiteDeveloper
Tuesday Mar 12, 2013 at 17:33 GMT

---

i'll look at it

[core-ai-bot]

Comment by peterflynn
Tuesday Mar 12, 2013 at 18:00 GMT

---

@WebsiteDeveloper just to be clear, we will need a code review from a Brackets committer also before landing this. But still feel free to review it -- an extra set of eyes to point out issues never hurts!

[core-ai-bot]

Comment by jbalsas
Tuesday Mar 12, 2013 at 18:39 GMT

---

Assigning to myself...

@WebsiteDeveloper, I'll be tagging along with you for this one. Please, feel free to review and voice your concerns, if any. I'll go over it myself later today or tomorrow and do the same, and then we'll iterate until everything looks good before merging, as usual ;)

[core-ai-bot]

Comment by WebsiteDeveloper
Tuesday Mar 12, 2013 at 19:03 GMT

---

@peterflynn i did know that, but as you said i'll just look at it
@jbalsas No issues spotted, all unit tests pass so should be fine.
After all, those are simple changes.

[core-ai-bot]

Comment by TomMalbran
Tuesday Mar 12, 2013 at 19:10 GMT

---

Simple, but could break anything if using the wrong forEach. Anyway I did tested everything and run the tests before submitting it.

There are still a couple of Object.keys().forEach iterations left. Those are OK, but for better consistency, should these be CollectionUtils.forEach loops?

[core-ai-bot]

Comment by WebsiteDeveloper
Tuesday Mar 12, 2013 at 19:16 GMT

---

As i'm a fan of consistency i would say yes, because generally consistency makes i a lot easier for new developers to understand the code, also it makes the code more maintainable.

[core-ai-bot]

Comment by jbalsas
Wednesday Mar 13, 2013 at 00:00 GMT

---

@TomMalbran I think we only have Object.keys().forEach in PreferencesManager and UrlParams. I'd also prefer to move them over to CollectionUtils.forEach for consistency.

Other than that, this is good to go.

[core-ai-bot]

Comment by TomMalbran
Wednesday Mar 13, 2013 at 01:21 GMT

---

@jbalsas Yes, those where the ones I was referring too, since I already replaced the Strings ones. I replaced those too. Should be good to go :)

[core-ai-bot]

Comment by jbalsas
Thursday Mar 14, 2013 at 18:56 GMT

---

@WebsiteDeveloper Thanks for your help with the review! Keep your eyes open, surely more like this are on their way ;)
@TomMalbran Sorry I couldn't get to this yesterday...

Looks good to me. Merging.

[core-ai-bot]

Comment by jbalsas
Thursday Mar 14, 2013 at 18:59 GMT

---

@TomMalbran By the way, I just realized that you're still using your personal fork of Brackets to create your pull requests. I think should be able to create branches directly in this main repo if that's an easier workflow for you.

[core-ai-bot]

Comment by TomMalbran
Friday Mar 15, 2013 at 00:36 GMT

---

Right. I think I am just used to do it like this, and since I am somehow still new to git I would need to learn how to do it.

And thanks for the review.