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

C19 Kunzite - Abby Castillo #69

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

abbymachines
Copy link

No description provided.

Copy link

@marciaga marciaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Overall, the translations from Python to JavaScript idioms went well. I'd suggest watching for opportunities to use const over let and invite you to explore alternative object and array iteration constructs such as .map(), .filter(), and .reduce()!

@@ -1,15 +1,131 @@
export const drawLetters = () => {
// Implement this method for wave 1
// initialize object to hold letter frequency table
const letterFreqs = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing wrong with it being defined here but because mappings like this can be valuable outside of being used in functions, it's often a better idea to define them in the module's main scope so that they can be exported and used elsewhere and in other functions in the same module!

// fill letterPool array with all the letters
const letterPool = [];

for (const [key, value] of Object.entries(letterFreqs)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job with this one, creating an array to hold all of the possible letter combinations with respect to their distribution!

}

// draw letters into hand array
let hand = [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since hand isn't reassigned, it should be a const.

for (let i = 0; i < 10; i++) {
let selector = Math.random() * letterPool.length;

let letter = letterPool.splice(selector, 1)[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job doubling up the array mutation and accessing the letter from the .splice() return value.

};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
let handCopy = lettersInHand;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you were trying to make a shallow copy here in an effort to avoid mutation, but just note that any mutation applied to handCopy will also apply to lettersInHand. If you want a shallow copy, you can instead use the spread operator:

const handCopy = [...lettersInHand];
handCopy.pop()
// now both handCopy and lettersInHand would be missing their last element.

Also, handCopy should be a const

}
}
}
let result = { word: largestWord, score: largestWordScore };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since result isn't reassigned, it should be a const.

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