finally done with adagrams js #86
Conversation
| @@ -1,15 +1,140 @@ | |||
| const LETTER_POOL = { | |||
There was a problem hiding this comment.
Great use of upper snake case for your constants!
| return keys[Math.floor(Math.random() * keys.length)]; | ||
| } | ||
|
|
||
| let drawnLetters = []; |
There was a problem hiding this comment.
Because drawnLetters is only used in the drawLetters function, I would make the variable local to the drawLetters function. Also, we add values to it, but we never reassign the reference to drawnLetters, so const may be the better choice here! Always prefer const over let. If you accidentally try to reassign a variable that was declared with const, then the JS interpreter will let you know and you can change it to let.
| }; | ||
|
|
||
|
|
||
| function getRandomProperty() { |
| } | ||
| } | ||
|
|
||
| return drawnLetters; |
There was a problem hiding this comment.
This function looks good but I have two suggestions:
- I would make a copy of LETTER_POOL because as it stands, you are destroying LETTER_POOL as you remove letters.
- You don't ever account for the different numbers of letters, so the randomness accounts for the 26 different keys, but it weights them each equally. To get around this, we would want to create some sort of structure that holds all the different letters and randomly picks one from there!
| export const usesAvailableLetters = (input, lettersInHand) => { | ||
| // Implement this method for wave 2 | ||
| }; | ||
| let bankCopy = JSON.parse(JSON.stringify(lettersInHand)) |
There was a problem hiding this comment.
This is a good way to copy a list. One thing to think about however is that the list we are copying simply contains letters, not objects, so a shallow copy using the spread operator will work fine here!
bankCopy = [...lettersInHand];
| if (!bankCopy.includes(letter.toUpperCase())){ | ||
| return false | ||
| } else if (bankCopy.includes(letter.toUpperCase())) { | ||
| bankCopy = bankCopy.filter(value => value !== letter) |
There was a problem hiding this comment.
The Javascript filter function runs in linear time. I wonder if there is a way to utilize objects to make this removal happen in constant time?
| // Implement this method for wave 3 | ||
| }; | ||
| let score = 0; | ||
| let lengthOfWord = word.length; |
There was a problem hiding this comment.
lengthOfWord never changes, so you could absolutely make this a const!
| }; | ||
| let score = 0; | ||
| let lengthOfWord = word.length; | ||
| const letterValueDict = { |
| return 0; | ||
| } | ||
|
|
||
| if(lengthOfWord >= 7){ |
There was a problem hiding this comment.
Best practice is to place a space after the if keyword to avoid looking like a function call!
| } | ||
| } | ||
|
|
||
| return { word: listOfMaxKeys[0], score: wordDict[listOfMaxKeys[0]] }; | ||
| }; |
There was a problem hiding this comment.
I understand the concept behind what you are trying to do here! Using objects to separate out each step makes sense, but it starts to get tedious and more complex when we do that. Since we are returning an object with word and score as keys, why not create the one object that has the first word and the first word score as values? From there, we can run through the rest of the wordList and make the comparisons we need in place! This will drastically decrease the complexity and code you need to use! An example is below:
export const highestScoreFrom = (words) => {
let winningWord = {
word: null,
score: null
}
for (let word of words){
let thisWord = {
word: word,
score: scoreWord(word)
}
if (thisWord.score > winningWord.score){
winningWord = thisWord
} else if (thisWord.score === winningWord.score){
const isBestTen = winningWord.word.length === 10;
const isWordTen = thisWord.word.length === 10;
const isWordShorter = thisWord.word.length < winningWord.word.length;
if ((isWordTen && !isBestTen) || (isWordShorter && !isBestTen)){
winningWord = thisWord;
}
}
}
return winningWord;
}
No description provided.