Zoisite - Whitney Shake #67
Conversation
There was a problem hiding this comment.
Nice work on js-adagrams!
I left a comment about the distribution of letters when you draw a random letter. Let me know if you have questions about it.
There are several of places that const would have been a better fit over let. When in doubt, use const and then swap to let when the JS interpreter complains. Additionally, I noticed you relied on the for-i loop throughout the project, but most of your loops could have used a for-of loop. When we don't need the index to access a value, we should use the for-of loop.
I also called out a couple of places where the conditional logic should be simplified which would increase code readability. What you have does work and passes the tests, but with so many nested conditionals that rely on continue, it was a little difficult to follow along without using the debugger to see what each line of code was executing and why. I suggest revisiting these areas and refactoring for practice.
Overall, good work on converting your python adagrams to javscript!
| @@ -1,15 +1,331 @@ | |||
| const LETTER_POOL = { | |||
There was a problem hiding this comment.
Nice job naming constant variables with all caps
| let lettersInHand = []; | ||
|
|
||
| while (lettersInHand.length < 10) { | ||
| let newNum = Math.floor(Math.random() * (26 + 1)); |
There was a problem hiding this comment.
instead of hardcoding 26, prefer to use a variable that describes what the value is or you could use letterArray.length too.
Also, newNum is not re-assigned in the while loop so you should use const
| let letterArray = Object.keys(LETTER_POOL); | ||
| let tempLetters = {}; | ||
| let lettersInHand = []; |
There was a problem hiding this comment.
These values are not re-assigned and should be declared with const. I tend to define everything with const and then switch them to let when the JS interpreter complains.
| if (currentLetter === undefined) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Instead of writing this condition and then just using continue, maybe you could add another condition to line 29 and then remove lines 26-28 completely.
So line 29 could be like:
if (currentLetter !== undefined and (!currentLetter in tempLetters)) {}
| else { | ||
| continue; |
| }; | ||
|
|
||
| export const highestScoreFrom = (words) => { | ||
| // Implement this method for wave 4 | ||
| let scoreArray = getScoreArray(words); |
There was a problem hiding this comment.
Most of the variables in this method should be declared with const since they are not reassigned.
| let tieBreaker = getTieBreaker(potentialWinners, lengthArray); | ||
| return {"word": tieBreaker, "score": highScore}; | ||
| } | ||
| }; |
There was a problem hiding this comment.
👍 I like the helper functions you wrote to use in highestScoreFrom. The tie breaking logic is pretty complex so breaking each part of the logic into helper functions keeps this function concise and readable.
| expectScores({ | ||
| '': 0, | ||
| }); |
| @@ -145,7 +147,8 @@ describe("Adagrams", () => { | |||
| const words = ["XXX", "XXXX", "X", "XX"]; | |||
| const correct = { word: "XXXX", score: scoreWord("XXXX") }; | |||
|
|
|||
| throw "Complete test by adding an assertion"; | |||
| // throw "Complete test by adding an assertion"; | |||
| expect(highestScoreFrom(words)).toEqual(correct); | |||
| return highScore; | ||
| }; | ||
|
|
||
| const getPotentialWinners = (words, scoreArray, highScore) => { |
There was a problem hiding this comment.
This method relies on the words array and the scoreArray to be the same length since they both use i to access the values. However relying on this fact and that the words and scores are in the same order can be tricky. If you have two lists holding words and their scores, could you use an object to associate words with their scores instead?
You can still iterate over the object to find the potentialWinners
No description provided.