Skip to content

Conversation

@dnabilali
Copy link

No description provided.

Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Looks good! I’ve added some suggestions, let me know if there's anything I can clarify ^_^

"Y",
"Z",
];
const lettersPoolDeepcopy = JSON.parse(JSON.stringify(lettersPool));

Choose a reason for hiding this comment

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

Performing a copy like this is a great place to use the spread operator! What could that look like?

Since lettersPool is declared inside the function, the data structure gets re-created every time we call the function. This means we shouldn't need to copy lettersPool to ensure that changes aren't persisted across calls.

Comment on lines +105 to +114
const hand = [];

for (let i = 0; i < 10; i++) {
const random_number = Math.floor(
Math.random() * lettersPoolDeepcopy.length
);
hand.push(lettersPoolDeepcopy[random_number]);
lettersPoolDeepcopy.splice(random_number, 1);
}
return hand;

Choose a reason for hiding this comment

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

Really nice approach!

Comment on lines +119 to +126
for (const letter of input) {
if (lettersInHand.includes(letter)) {
const index = lettersInHand.indexOf(letter);
lettersInHand.splice(index, 1);
} else {
return false;
}
}

Choose a reason for hiding this comment

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

Nice approach! If we needed to reduce the time complexity of our solution, another approach could be to build a frequency map of our hand then loop over the characters in the input, checking if the character is in our frequency map, and if it is, then check the value to see if there are still tiles left in our hand for that letter.

const index = lettersInHand.indexOf(letter);
lettersInHand.splice(index, 1);
} else {
return false;

Choose a reason for hiding this comment

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

I like the early exit =]


export const scoreWord = (word) => {
// Implement this method for wave 3
const lettersValue = {

Choose a reason for hiding this comment

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

I like keeping these in the function that uses them so they're close to where they are referenced.

};
let score = 0;
word = word.toUpperCase();
for (const letter of word) {

Choose a reason for hiding this comment

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

Nice use of a for...of loop.

Comment on lines +161 to +163
for (const letter of word) {
score += lettersValue[letter];
}

Choose a reason for hiding this comment

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

This could have an unexpected result if the string word has characters that aren't in lettersValue. If we have an input like "ABC DEF" then score will hold NaN (Not a Number) after the loop. If we wanted to skip non-alphabetic characters or characters that aren't in lettersValue, how could we do that?

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