C22- Phoenix- Amber Edwards#23
Conversation
Ported highestScoreFrom()
Remove lettersUsed object to improve space complexity.
Follow single responsibility principle
Following feedback from python version to keep variables separated until the return line, to minimize typos.
Used feedback from python PR to change function logic.
scoreWord now correctly handles strings of just whitespace. useAvailableLetters was refactored to have better space and time complexity.
| import { fileURLToPath } from "node:url"; | ||
| import js from "@eslint/js"; | ||
| import { FlatCompat } from "@eslint/eslintrc"; | ||
| import eslintConfigPrettier from "eslint-config-prettier"; |
There was a problem hiding this comment.
Did you find that adding this resulted in less fighting with prettier? Personally, I prefer to use the default VS Code Js Formatter.
| const MIN_BONUS_LENGTH = 7 | ||
| const LENGTH_BONUS = 8 | ||
|
|
||
| const createWeightedLetterPool = () =>{ |
There was a problem hiding this comment.
Nice helper to expand the letter weight information into a pool that actually has the appropriate number of copies. Since this will get used in each call to drawLetters, we're free to modify the result as well if need be, since the next call will get its own fresh copy.
| const createWeightedLetterPool = () =>{ | ||
| let weightedLetterPool = []; | ||
| for (const [letter, freq] of Object.entries(LETTER_POOL)){ | ||
| for (let i=0; i<freq; i++){ |
There was a problem hiding this comment.
Nit: Include spaces around binary operators (and throughout).
for (let i = 0; i < freq; i++){| const hand = []; | ||
| const weightedLetterPool = createWeightedLetterPool() | ||
|
|
||
| while (hand.length < HAND_SIZE){ |
There was a problem hiding this comment.
Nit: Leave a space between the conditional and the open of the block (and throughout).
while (hand.length < HAND_SIZE) {There was a problem hiding this comment.
I'm still a bit on the fence about whether I prefer the while approach or a for loop. Since you're guaranteed to pick a letter each time through the loop, we know that we only need to run HAND_SIZE times, but from the use of the while loop, it's less clear from the condition that this will only run that many times. For that reason, I'd probably lean more towards a for loop.
| let randomIdx = Math.floor(Math.random()*weightedLetterPool.length); | ||
| let randomLetter = weightedLetterPool[randomIdx]; | ||
| hand.push(randomLetter); | ||
| weightedLetterPool.splice(randomIdx,1); |
There was a problem hiding this comment.
Much like pop/remove in python, this will need to shift values forward to close the hole left by removed item. To avoid the internal loop this requires, we can swap it with value to the end of the pool, then pop it. Strictly speaking, it's not even necessary to remove the value from the list, so lang as we adjusted the value we used as the list length to reflect the size of the available portion of the pool.
| let highScoreWords = [] | ||
| for (let word of words){ | ||
| if(scoreWord(word) === maxScore){ | ||
| highScoreWords.push(word); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This is a place where JS programmers might choose to use Array.filter
const highScoreWords = words.filter(word => scoreWord(word) === maxScore);| let maxScore = getMaxScore(words); | ||
| let highScoreWords = getHighScoreWords(words, maxScore); |
There was a problem hiding this comment.
Working with only those words having the highest score simplifies the rest of the logic in this function.
|
|
||
| for (let word of highScoreWords){ | ||
| if (winningWord.length === 10){ | ||
| return {word: winningWord, score: maxScore}; |
There was a problem hiding this comment.
As soon as we find a word that is 10 letters, we know nothing else could win, so we can exit here. Rather than duplicating the return, we could break out of the loop.
| if (winningWord.length === 10){ | ||
| return {word: winningWord, score: maxScore}; | ||
| }; | ||
| if (winningWord.length > word.length || word.length === 10){ |
There was a problem hiding this comment.
I would probably turn this around so that it reads if the new word is shorter, rather than if the old word is longer.
if (word.length < winningWord.length || word.length === 10){This keeps the two conditions more consistent.
| for (let word of highScoreWords){ | ||
| if (winningWord.length === 10){ | ||
| return {word: winningWord, score: maxScore}; | ||
| }; |
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
(Sorry if GH was sending notifications about comments along the way. It looks like it fell back to single comment mode at some point! 🙏)
2 participants
No description provided.