Sea Turtles JS-Adagrams -Tyrah G. #103
Conversation
There was a problem hiding this comment.
Nice work.
Your overall approaches are good, but really pay attention to those details. For instance, drawLetters currently returns the same letters with every call. I definitely see your intention, but be sure to double check your code. The tests only check for impossible hands, but it shows up in the demo game. usesAvailableLetters also has an issue due to how the hand is being used. Also, think about how your variable names imply what they contain. Like python, JS variables can refer to anything, so giving care to how we name them can make our code easier to understand and maintain.
But overall, good job.
| @@ -0,0 +1,57 @@ | |||
| export const LETTER_POOL = { | |||
There was a problem hiding this comment.
Nice way to avoid cluttering up the logic file with these massive constants!
| let letterPool = []; | ||
| for (const [key, value] of Object.entries(LETTER_POOL)) { | ||
| for (let i = 0; i < value; i++) { | ||
| letterPool.push(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Great way to build up the pool of tiles to draw from.
Since we modify only the contents of letterPool, it could still be declared with const since we never reassign the variable.
| export const drawLetters = () => { | ||
| // Implement this method for wave 1 | ||
| let letterPool = createLetterPool(); | ||
| let letterPoolDeepCopy = JSON.parse(JSON.stringify(letterPool)); |
There was a problem hiding this comment.
Since you build a new `letterPool' whenever this function is called, there's no need to make another copy.
| function shuffleArray(letterPoolDeepCopy) { | ||
| for (let i = 0; i < letterPoolDeepCopy; i--) { | ||
| const j = Math.floor(Math.random() * (i + 1)); | ||
| [array[i], array[j]] = [array[j], array[i]]; | ||
| } | ||
| } |
There was a problem hiding this comment.
This function is defined, but never called. As a result, every hand we get is always A A A A A A A A A B. We now have scrabble for 🐑! 😁
I'd suggest moving this out of this function since it doesn't really need to be defined inside the drawLetters function. That would also make it easier to see that it wasn't being used yet, which is really the only issue here.
| [array[i], array[j]] = [array[j], array[i]]; | ||
| } | ||
| } | ||
| let letterBank = letterPoolDeepCopy.slice(0, 10); |
There was a problem hiding this comment.
This is a great way to get the first 10 items out of the list.
We can declare letterBank with const.
| if (lettersInHand.includes(char) === false) { | ||
| return false; | ||
| } else if (lettersInHand.includes(char)) { | ||
| lettersInHand.splice(char[letters], 1); |
There was a problem hiding this comment.
This isn't working properly. It appears to because the letters in the word and hand apparently line up in the tests.
But really, splice takes a position as its first argument. char will be a single-letter string (we looked it up from the word) and any index lookup greater than position 0 (so anything but the first letter would be passing in undefined). But whether a letter or undefined, it's having the effect of removing the first element from the list.
We can see this with this example
usesAvailableLetters('cat', ['t', 'a', 'c'])which should be true, but returns false.
So we would need to find the position of a letter to splice out of the array before doing the splice. Check out indexOf as an alternative to includes.
Also, we don't want to modify that hand that was passed in. So if we're doing to take a destructive (to the hand) approach here, we would want to make a local copy before modifying it.
| if (input.toUpperCase() in SCORE_CHART) { | ||
| score += SCORE_CHART[input.toUpperCase()]; | ||
| } |
There was a problem hiding this comment.
Nice thought to add this bit of additional safety.
It wasn't a requirement, though in the context of these functions, we might expect to only receive words that are made of valid letter tiles.
Often, when there are a bunch of functions working together in a larger system, we'll expect the validation to have been at one central point so that we don't need to think about so many edge cases in the rest of our code.
But nice overall approach of iterating over the word, looking up the score of each letter, and adding the bonus if needed.
| } else if (wordScore === maxScore) { | ||
| let wordLen = input.length; | ||
| let highScoreWordLen = highScoreWord.length; | ||
| { |
There was a problem hiding this comment.
No need to introduce a block here.
| let maxScore = 0; | ||
| let highScoreWord = ""; | ||
|
|
||
| for (let word = 0; word < words.length; word++) { |
There was a problem hiding this comment.
Nice job. Your cascading checks are pretty easy to follow!
Here, consider a iteration variable name that indicates that this is numeric (word_pos, or even simply i). word sounds like it should contain a string.
| expectScores({ | ||
| "": 0, | ||
| }); |
Still fixing wave 3