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

Made showHints work for lots of windows. #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cfraizer
Copy link

@cfraizer cfraizer commented Sep 7, 2018

The function showHints relied on a feature of UnderscoreJS that apparently
LoDash doesn't support. (The callback for groupBy assumed that the index of
the current element would be passed as its second argument. Underscore does
this, but LoDash apparently does not.)

I'm not a great JS programmer. I ended up changing the partitioning to use
JavaScript's built-in Array.reduce and Object.values functions instead of
UnderscoreJS's (now LoDash's) _.groupBy and _.toArray.

The function showHints relied on a feature of UnderscoreJS that apparently
LoDash doesn't support. (The callback for groupBy assumed that the index of
the current element would be passed as its second argument.  Underscore does
this, but LoDash apparently does not.)

I'm not a great JS programmer. I ended up changing the partitioning to use
JavaScript's built-in Array.reduce and Object.values functions instead of
UnderscoreJS's (now LoDash's) _.groupBy and _.toArray.
@purag
Copy link
Owner

purag commented Sep 30, 2018

Hi Colin! Thanks for the contribution.

I actually think we can do something simpler than this. Since lodash no longer passes the index as the second parameter, we'll just have to keep track of it ourselves. This is actually fairly easy thanks to closures in JavaScript. Instead of reworking the whole solution, we can continue to use the convenient lodash functions and just keep track of k like this:

let k = 0;
let lists = _.toArray(_.groupBy(windows, () => k++ % HINT_CHARS.length));

If you want to go ahead and update your pull request with this new method I'd gladly merge it!

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.

2 participants