-
Notifications
You must be signed in to change notification settings - Fork 710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kate - Lion - js-adagrams #44
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent job! There are some comments below on places to make the code cleaner/simpler. For commits, it would be great if the commit messages could be more descriptive. Instead of talking about what tests are passing, something like "Added drawLetters function" would be great. Good job!
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
expectScores({ | ||
"": 0, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
@@ -1,15 +1,122 @@ | |||
const letterDist = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
Y: 2, | ||
Z: 1, | ||
}; | ||
|
||
export const drawLetters = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
for (let letter of input) { | ||
if (lettersInHand.includes(letter)) { | ||
let i = lettersInHand.indexOf(letter); | ||
lettersInHand.splice(i, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be removing letters from the lettersInHand
array. This function should just be checking to make sure all of the letters in input
is in lettersInHand
. If the user is losing letters every time we do this check, there will be no hand left soon.
let letterScore = {}; | ||
const buildScoreDict = (letters, score) => { | ||
for (const letter of letters) { | ||
letterScore[letter] = score; | ||
} | ||
}; | ||
|
||
buildScoreDict(["A", "E", "I", "O", "U", "L", "N", "R", "S", "T"], 1); | ||
buildScoreDict(["D", "G"], 2); | ||
buildScoreDict(["B", "C", "M", "P"], 3); | ||
buildScoreDict(["F", "H", "V", "W", "Y"], 4); | ||
buildScoreDict(["K"], 5); | ||
buildScoreDict(["J", "X"], 8); | ||
buildScoreDict(["Q", "Z"], 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done by just creating the object like you do with the letterDist
.
word = word.toUpperCase(); | ||
|
||
let points = 0; | ||
if (word.length === 0) { | ||
return points; | ||
} else if (word.length > 6) { | ||
points += 8; | ||
} | ||
for (let letter of word) { | ||
points += letterScore[letter]; | ||
} | ||
return points; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
// Implement this method for wave 4 | ||
let max = 0; | ||
let highestScoreWord = ""; | ||
for (let word of words) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of calling scoreWord
multiple times, it would be more efficient to call it once at the beginning of the for loop and store it in a variable.
} else if (word.length != 10 && highestScoreWord.length === 10) { | ||
highestScoreWord = highestScoreWord; | ||
} else if (word.length === highestScoreWord) { | ||
highestScoreWord = highestScoreWord; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the highestScoreWord
is already the highestScoreWord
, these else if statements can be removed.
No description provided.