-
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
sea turtles - jae c17 #37
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.
Nice job!
Your approaches are generally clean and easy to follow. You're making good use of helper data structures to avoid additional looping. There's a small problem with the 10-letter word handling in highestScoreFrom
, but overall it looks good. And while JS isn't quite as opinionated about indenting as python is, still keep it in mind and focus on keeping your code readable with good use of indentation and line wrapping.
Great work!
const letter = Array.from(Object.keys(LETTER_POOL)); | ||
const vals = Array.from(Object.values(LETTER_POOL)); | ||
|
||
const letter_list = [] | ||
let i = 0 | ||
for (i = 0; i < letter.length; i++){ | ||
letter_list.push(...letter[i].repeat(vals[i])) | ||
}; |
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.
Rather thank making these two separate but related lists, we could iterate over the key value pairs as follows.
const letter_list = []
for (for const [letter, val] of Object.entries(LETTER_POOL)){
letter_list.push(...letter.repeat(val))
}
Also, the ;
after a loop/condition block {}
is unnecessary.
const numbers = new Set(); | ||
while (numbers.size !== 10) { | ||
numbers.add(Math.floor(Math.random() * letter_list.length)); | ||
} |
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.
Great way of getting a unique set of indices to pick.
|
||
let hand = [] |
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 we're only modifying the contents, never reassigning the variable, this could be declared const
.
let hand = [] | ||
for (const num of numbers){ | ||
hand.push(letter_list[num]) | ||
}return hand; |
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.
Watch your formatting. JS doesn't use whitespace for indicating control flow, but we should still pay attention to spacing and indentation for readability.
let hashInput = {}; | ||
let valid = [] | ||
|
||
for (const letter of lettersInHand){handHash[letter] = handHash[letter] ? handHash[letter] + 1 : 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.
Consider making a helper method to build a frequency map from an array/string. Then we could use it both for the hand and the word!
And take care with the wrapping here. I would expect the block body to be on the next line.
if (word.length == 0){ | ||
return 0; | ||
} | ||
let results = wordInfo(word).score; |
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.
With your helper, this is now a very short function.
if(word.length == 10){ | ||
return wordInfo(word); | ||
} |
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.
Just because the word is ten letters long, that doesn't mean it's the winner. "EEEEEEEEEE" would score 18 (10 + bonus), but "QUIZ" would score 22.
|
||
// find max score & sort words by length | ||
const winner = Math.max(...scores); | ||
const word_sort = words.sort((a,b) => a.length - b.length); |
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.
Nice custom sort comparator! We'd want to be sure that the sort method is stable, because otherwise we wouldn't be sure which is the shortest word we see first!
for (const word of word_sort){ | ||
let word_dict = wordInfo(word); | ||
if (word_dict.score == winner){ | ||
allBest.push(word_dict)}; |
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 is essentially filtering the words to those with the max score. This is also where we'd want to check for the length being ten, since if we do find a ten letter word here, then we know it's got the high score and is ten letters. A stable sort would mean it would have remained in the proper order relative to other words of the same length, so the first word of length 10 we see would definitely be a winner!
Otherwise, as you've written, it would be the first, shortest word that had the max score.
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.
Nice use of the helper.
No description provided.