AC2-Ocelots - Catherine Bandarchuk - JS-AdaGrams - #7
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Looks good! I’ve added some suggestions, let me know if there's anything I can clarify ^_^
| let availableListOfLetters = []; | ||
| for (const letter of POOL_LETTERS_DICT) { | ||
| for (let i = 0; i < letter["count"]; ++i) { | ||
| availableListOfLetters.push(letter["letter"]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Really nice solution for ensuring we get the desired distribution of letters.
| @@ -1,15 +1,136 @@ | |||
| export const drawLetters = () => { | |||
| // Implement this method for wave 1 | |||
| const POOL_LETTERS_DICT = [ | |||
| let lettersInHand = []; | ||
| for (let i = 0; i < 10; ++i) { | ||
| let randomIndexLetter = Math.floor( | ||
| Math.random() * availableListOfLetters.length | ||
| ); | ||
| let oneLetter = availableListOfLetters[randomIndexLetter]; | ||
| availableListOfLetters.splice(randomIndexLetter, 1); | ||
| lettersInHand.push(oneLetter); | ||
| } | ||
|
|
||
| return lettersInHand; |
| let randomIndexLetter = Math.floor( | ||
| Math.random() * availableListOfLetters.length | ||
| ); |
There was a problem hiding this comment.
Unless we're in a system that is really low on memory, I would consider making a new variable to hold the result of the multiplication over indentation.
const randomFloat = Math.random() * availableListOfLetters.length;
const randomIndex = Math.floor(randomFloat);| let randomIndexLetter = Math.floor( | ||
| Math.random() * availableListOfLetters.length | ||
| ); | ||
| let oneLetter = availableListOfLetters[randomIndexLetter]; |
There was a problem hiding this comment.
I suggest using const to declare variables which shouldn't get reassigned within the current scope.
| for (let letter of input) { | ||
| const indexOfLetter = lettersInHand.indexOf(letter); | ||
| if (indexOfLetter !== -1) { | ||
| lettersInHand.splice(indexOfLetter, 1); | ||
| checkedWord.push(letter); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| export const scoreWord = (word) => { | ||
| // Implement this method for wave 3 | ||
| let user_points = 0; | ||
| const SCORE_CHART = [ |
There was a problem hiding this comment.
Since this is a local constant (only has scope in this function), we should use camel case for the naming scoreChart.
| for (let letterW of word) { | ||
| const letterObj = SCORE_CHART.find(({ letter }) => letter === letterW); | ||
| user_points += letterObj.value; | ||
| } |
There was a problem hiding this comment.
The array find function is an O(n) operation. If we structured SCORE_CHART as a javascript object where the letters are the keys and the points are the values, you could reduce the time complexity by being able to access the SCORE_CHART's values using the letters of the input word as keys.
const scoreChart = {
A: 1,
E: 1,
I: 1,
...
X: 8,
Q: 10,
Z: 10,
};
for (const letter of word) {
const letterValue = letterValues[letter];
if (letterValue != undefined) {
score += letterValue;
}
}| if (word.length >= 7) { | ||
| user_points = 8; | ||
| } | ||
| for (let letterW of word) { |
There was a problem hiding this comment.
Nice use of a for...of loop.
| let usersScore = 0; | ||
| let maxScore = 0; | ||
| let maxWord = ""; | ||
| for (let word of words) { |
There was a problem hiding this comment.
Nice implementation to tie break in a single loop!
2 participants
Didn't do optional wave 5.