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

Stockticker refactored #10

Merged
merged 3 commits into from
Sep 22, 2018

Conversation

taranvohra
Copy link
Contributor

No description provided.

@markerikson
Copy link
Collaborator

Looks really good! The only issue I have is that <Slice> is now issuing the updates, which means that we'll have 4 * NUM_SLICES updates being triggered each time around rather than just 4. Also, part of the point of the exercise is to have varying slices being updated each time, rather than the same slices.

What I'd like to see is a random slice and a random ID in that slice being updated each time. Tell you what - how about we move the randomization logic into a thunk or something in index.js, and have the timers do this:

function updateRandomEntry() {
    // calculate random slice
    // calculate random ID
    store.dispatch(updatePair(slice, id));
}

setInterval(updateRandomEntry, 50);

@taranvohra
Copy link
Contributor Author

Good idea. I moved the update logic to index.js so the decision as to which slice and pair to update is randomized now

@markerikson markerikson merged commit d96dbad into reduxjs:master Sep 22, 2018
@markerikson
Copy link
Collaborator

Thanks, appreciate it!

If you can go ahead and do the same for the others (ideally today), that'd be great.

@markerikson
Copy link
Collaborator

So after merging this, I just realized there's an issue:

function updateRandomPairInSlice() {
    const sliceId = Math.floor(Math.random() * c.NUMBER_OF_SLICES) + 1;
    const pairId = Math.floor(Math.random() * (c.NUM_ENTRIES / c.NUMBER_OF_SLICES)) + 1;
    store.dispatch(updatePair(sliceId, pairId));
}

The slices themselves are going to be numbered 0...N-1 because of the reduce() call, but this is going to generate slice IDs of 1...N. So, slice 0 will never get updated, and actions with a slice ID of N will result in no update at all. The + 1 values need to be removed, or at least the slice one does (not sure on the math for the pair ID yet).

I'll tweak that and push the change.

@taranvohra
Copy link
Contributor Author

taranvohra commented Sep 22, 2018

Right. removing the +1 would solve the problem generating id's from 0..N-1. I think the same fix should suffice for pairId because adding a 1 would make pairId 0 not update at all

@markerikson
Copy link
Collaborator

Yep, just fixed that and pushed.

Aslemammad pushed a commit to Aslemammad/react-state-benchmarks that referenced this pull request May 30, 2021
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

2 participants