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

Fix: result's index should be original list index #10

Closed
wants to merge 5 commits into from

Conversation

dumin
Copy link

@dumin dumin commented Jul 28, 2017

when i try this example: http://moonjs.ga/examples/search/index.html
i tap 'catch' but result list is not what i want

because after pipeline, some item may be removed because of zero length, so the index in result is not the original list index

so we can record index map (if need) to fix this problem

@kbrsh
Copy link
Owner

kbrsh commented Jul 28, 2017

This looks good, but I think that this can be accomplished in a simpler manner. Wade.process returns false if the entry has a length of 0. The indexing doesn't have a clue, so I think we can use a sparse array, and if the value is undefined, then then skip over it, that way the index is correct.

@dumin
Copy link
Author

dumin commented Jul 28, 2017

yes, method you mentioned is better

@kbrsh
Copy link
Owner

kbrsh commented Jul 28, 2017

Will you be updating the PR, or should I get to work on this?

@dumin
Copy link
Author

dumin commented Jul 28, 2017

I think i hava done, but not sure my PR update operation is correct

@kbrsh
Copy link
Owner

kbrsh commented Jul 28, 2017

It looks good, but when filling normalizedData you can just do something like:

if(item !== false) {
  normalizedData[i] = item;
}

This is because when initializing an array with new Array(), all of the elements are already undefined.

@kbrsh
Copy link
Owner

kbrsh commented Jul 29, 2017

Alright, since this has been inactive for a while, I'll try implementing it. Thanks for the help!

@kbrsh kbrsh closed this in 6c52927 Jul 29, 2017
@dumin
Copy link
Author

dumin commented Jul 30, 2017

Set new Array(len) just to tell the compiler the initial size of array, to avoid memory copy in the dynamic expansion
I used to define every elements of this array, otherwise it would be skipped when use forEach / map function, this may cause other users to be troubled

E.g:

arr = new Array(10)
arr[4] = undefined
arr[5] = 'hello'

// forEach only touch element  4 & 5

@kbrsh
Copy link
Owner

kbrsh commented Jul 30, 2017

Yeah, I know what new Array does. In this case you shouldn't really be needing to access the normalized data though. When accessed, it has a value of undefined. That's why I suggested merely skipping over it.

@leeoniya
Copy link

@kingpixil new is unnecessary; Array(len) works fine ;)

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.

None yet

3 participants