Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replaces use of indexOf with more idomatic CoffeeScript #501

Merged
merged 2 commits into from
Oct 16, 2013

Conversation

schliffer
Copy link
Contributor

Removes all use of Array.prototype.indexOf to support IE8

@schliffer
Copy link
Contributor Author

@adambiggs

@aeischeid
Copy link
Member

Is your main goal to get support for IE<9 or to make Spine more clear/consistent?

I find indexOf to be more clear than using 'in', but that may just be a personal preference. What do others think?

As for the IE<9 thing. My general stance is if there are shims that deal with that easily enough then I don't want to let deprecated browser implementations influence the library any more than they have to.

@schliffer
Copy link
Contributor Author

I'm trying to accomplish both. I think using in is far more clear than indexOf for situations where you do not need the index for some other reason. For cases where you need the index to then remove the item from the list using splice, I think the Coffee list comprehension is just as clear and probably more idiomatic. And I definitely think that

if ev in evts
  evts = (e for e in evts when e isnt ev)

is more clear than

if ~(i = evts.indexOf(ev))
  evts.splice(i, 1)

There is no real reason to use ~ combined with indexOf when in is available.

The change to list.coffee is probably uglier but that change is more about IE8 than about clarity.

@adambiggs
Copy link
Member

@aeischeid Personally i think in is more clear & concise.

@jschliffer can you commit the JS as well please.

Interesting, looks like indexOf is quite a bit slower than simply finding the index using a loop.

@aeischeid
Copy link
Member

if ~(i = evts.indexOf(ev)) is definitely a bit odd, but evts = (e for e in evts when e isnt ev) is a lot more to digest than evts.splice(i, 1)

Oh well, I can go with the flow here. It is pretty interesting that looping is faster. That would have been my next minor objection, that index of is probably faster, but that is the opposite of what I expected. Given that I'll just spin it for myself that we are doing this for a performance improvement ;)

aeischeid added a commit that referenced this pull request Oct 16, 2013
replaces use of indexOf with more idomatic CoffeeScript
@aeischeid aeischeid merged commit f202873 into spine:dev Oct 16, 2013
@schliffer schliffer deleted the remove_indexOf branch October 16, 2013 18:10
@jamiter
Copy link
Contributor

jamiter commented Oct 16, 2013

Just a detail, but shouldn't this loop use a break?

for item, idx in @items
  index = idx if item is @current

Like:

for item, idx in @items when item is @current
  index = idx
  break

Just a little performance thingy ;)

@adambiggs
Copy link
Member

Yes it should! Nice catch :)

adambiggs pushed a commit that referenced this pull request Oct 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants