Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
🎉 Nice job tackling adagrams using JavaScript! I've left comments throughout, so please take a look and let me know in Slack or our next 1:1 if there's anything you'd like to follow up on. Take a look at usesAvailableLetters in particular.
| export const drawLetters = () => { | ||
| // Implement this method for wave 1 | ||
| }; | ||
| const letter_count = { |
There was a problem hiding this comment.
Consider defining letter_count outside the function to focus on the code. Your code already does the work of making a copy of data when building the expanded letter list, so relocating it won't be a problem.
| const drawn = []; | ||
|
|
||
| const keyList = Object.keys(letter_count); | ||
| for(let key of keyList) { |
| let frequency = 0; | ||
| while(frequency < letter_count[key]){ | ||
| letter_bank.push(key) | ||
| frequency ++ | ||
| }; |
There was a problem hiding this comment.
We know how many times this loop should run, so prefer using a for loop to a while loop.
for (let i = 0; i < letter_count[key]; i += 1) {
letter_bank.push(key)
}; | }; | ||
|
|
||
| for(let i = 0; i < 10; i++) { | ||
| let select_char = Math.floor(Math.random() * (letter_bank.length -1)); |
There was a problem hiding this comment.
random is exclusive on the upper end, so subtracting 1 from the size will never pick the final location. Also, prefer const here.
const select_char = Math.floor(Math.random() * (letter_bank.length));This logic selects letters with equal weighting. You have logic to reject invalid letters (letters we've already used up), but be aware that this will tend to select hands with an overrepresentation of "hard" letters, which would make it harder for the player to form words.
| for(let i = 0; i < 10; i++) { | ||
| let select_char = Math.floor(Math.random() * (letter_bank.length -1)); | ||
| drawn.push(letter_bank[select_char]); | ||
| letter_bank.splice(select_char, 1); |
There was a problem hiding this comment.
👀splice like python's remove is linear with respect to the length of the list. This won't be a problem for the size of data we're working with, but consider using a strategy of virtually "dividing" the list into a used and unused side. Swapping a value to the used side would be constant time!
|
|
||
| let total_score = 0 ; | ||
|
|
||
| for(let rawChar of word) { |
There was a problem hiding this comment.
for isn't a function, so the () should stick directly to it. Leave a space after for and other control structure keywords (like if and while)
| let total_score = 0 ; | ||
|
|
||
| for(let rawChar of word) { | ||
| let char = rawChar.toUpperCase(); |
There was a problem hiding this comment.
const again. Generally always try to use const and only switch back to let if JS tells us we have to (if we really do need to change what the variable refers to).
|
|
||
| for(let rawChar of word) { | ||
| let char = rawChar.toUpperCase(); | ||
| if(Object.keys(score_card).includes(char)){ |
There was a problem hiding this comment.
👀 This will iterate over all the keys of the score chart (building a new list), then iterating through the list to look for char. Instead, we can use some variation of in, or hasOwnKey to lookup a key in an object. Or, we can try to access a key, and use falsiness (from undefined) to provide a default value such as 0.
| let score = scoreWord(each_word) | ||
| if (score in scoreObject) { | ||
| scoreObject[score].push(each_word) | ||
| } | ||
| else { | ||
| scoreObject[score] = [each_word] | ||
| } |
There was a problem hiding this comment.
👍 Nice way to build up lists of tied words!
| if (scoreObject[maxScore].length > 1) { | ||
| if (wordToChoose.length !== 10) { | ||
| for (let each_word of scoreObject[maxScore]) { | ||
| if (each_word.length < wordToChoose.length) { |
There was a problem hiding this comment.
It's a little surprising to no have the 10-letter check here as well, but the logic checks out. If the first word is 10-letter, we'll never get here. If any other word is 10-letter, we'll exit the loop. Consider teasing apart some of this logic for clarity.
2 participants
No description provided.