G Quinn Kunzite JS Adagrams #83
Conversation
There was a problem hiding this comment.
Nice work! Overall, the translations from Python to JavaScript idioms went well. I'd suggest watching for opportunities to use const over let, use correct syntax conventions (spaces around assignment operators, putting code appropriately on new lines, using semicolons, and using spaces after object key: value assignments.
| } | ||
|
|
||
|
|
||
| letters.push(key); |
There was a problem hiding this comment.
If you moved this into the if block, you wouldn't need the continue. I think that reads better. Also use caution with line spacing (one line space is really the maximum) and remember that there's spacing around the else, e.g.
if (someCond) {
// do stuff
} else {
// do something else
}| }; | ||
|
|
||
| export const usesAvailableLetters = (input, lettersInHand) => { | ||
| // Implement this method for wave 2 | ||
| const lettersInHandCopy = [...lettersInHand]; |
There was a problem hiding this comment.
Nice, this is a great way to make a shallow copy, which is all we need here!
| for (const letter of input){ | ||
| if (!lettersInHandCopy.includes(letter)){ | ||
| return false; | ||
| } const index = lettersInHandCopy.indexOf(letter); |
There was a problem hiding this comment.
this const declaration should be on a new line
| const lettersInHandCopy = [...lettersInHand]; | ||
| for (const letter of input){ | ||
| if (!lettersInHandCopy.includes(letter)){ | ||
| return false; |
| if (!lettersInHandCopy.includes(letter)){ | ||
| return false; | ||
| } const index = lettersInHandCopy.indexOf(letter); | ||
| let temp = lettersInHandCopy[lettersInHandCopy.length - 1] |
There was a problem hiding this comment.
temp isn't reassigned so it should be a const, but looking at the next few lines of code, shifting elements in an array around just to use pop(), while it works, is less ideal than using the array method .splice(). You could just use the index you have to to splice out the element from the array like so:
lettersInHandCopy.splice(index, 1);and skip the element shifting! Remember simpler is almost always better!
| points.push(0) | ||
| } | ||
|
|
||
| let newWord = word.toUpperCase(); |
There was a problem hiding this comment.
Since this variable isn't reassigned, you should use const over let.
| let total = points.reduce((sum, num) => sum + num); | ||
| console.log(total); | ||
| if (word.length >= 7){ | ||
| (total += 8); |
There was a problem hiding this comment.
This expression shouldn't have parentheses around it
| } | ||
| } | ||
| console.log(points) | ||
| let total = points.reduce((sum, num) => sum + num); |
There was a problem hiding this comment.
Good use of reduce to sum an array of numbers, but consider whether an array is the best data structure (or whether a structure is even necessary. You could just start by assigning total to 0, then in the for-loop on line 107, add the score to total. That approach is more straightforward and doesn't involve the additional iteration that .reduce() does.
| @@ -120,7 +120,9 @@ describe("Adagrams", () => { | |||
| }); | |||
|
|
|||
| it("returns a score of 0 if given an empty input", () => { | |||
| throw "Complete test"; | |||
| expectScores({ | |||
| '':0 | |||
| @@ -145,7 +147,7 @@ describe("Adagrams", () => { | |||
| const words = ["XXX", "XXXX", "X", "XX"]; | |||
| const correct = { word: "XXXX", score: scoreWord("XXXX") }; | |||
|
|
|||
| throw "Complete test by adding an assertion"; | |||
| expect(highestScoreFrom(words)).toEqual(correct); | |||
No description provided.