-
Notifications
You must be signed in to change notification settings - Fork 20
Ocelots - Lisa Utsett #11
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
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.
Looks good! I’ve added some suggestions, let me know if there's anything I can clarify ^_^
| const letterPool = { | ||
| A: 9, B: 2, C: 2, D: 4, | ||
| E: 12, F: 2, G: 3, H: 2, | ||
| I: 9, J: 1, K: 1, L: 4, | ||
| M: 2, N: 6, O: 8, P: 2, | ||
| Q: 1, R: 6, S: 4, T: 6, | ||
| U: 4, V: 2, W: 2, X: 1, | ||
| Y: 2, Z: 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.
Tiny style nitpick: This is not a typical structure we see for objects/dictionaries. It takes up less lines, but we need to carefully read each line to be aware of what keys/values are on each. Though it takes more space, I suggest dropping each key/value pair to a new line to make them very easy and quick to read, unless you are on a team that has expressed a different styling that should be used.
| let bagOfLetters = []; | ||
| for (let letter in letterPool) { | ||
| for (let i = 0; i < letterPool[letter]; ++i) { | ||
| bagOfLetters.push(letter); | ||
| } | ||
| } | ||
| const drawnLetters = []; |
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.
Really nice solution for ensuring we get the desired distribution of letters.
|
|
||
| // adds ten random letters to hand | ||
| for (let i = 0; i < 10; ++i) { | ||
| let randomIndex = Math.floor((Math.random() * bagOfLetters.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.
This line is getting a little nested, it isn't necessary, but I would consider moving the multiplication to its own line to make it even easier to read quickly.
|
|
||
| export const highestScoreFrom = (words) => { | ||
| // Implement this method for wave 4 | ||
| lettersInHand.forEach(createMap); |
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.
Cool use of a nested function to build the frequency map!
| // if letter is there BUT there are available letters left, decrement the frequency of the letter by 1 | ||
| // if letter is not there, return false | ||
| for (let i = 0; i < input.length; ++i) { | ||
| if (lettersInHand.includes(input[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.
Something to consider for complexity, .includes does a linear search of the object keys and will be O(n) where n is the total number of keys. If we want that O(1) access, we could do something like:
letterValue = lettersInHand[input[i]]
if (letterValue === undefined || letterValue === 0) {
return false;
}
letterFreqMap[input[i]]--;| let score = 0; | ||
| if (word.length > 0) { | ||
| for (let letter in word){ | ||
| score += scoreChart[word[letter].toUpperCase()]; | ||
| } | ||
| if (word.length >=7){ | ||
| score += 8; | ||
| } | ||
| } | ||
| return 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.
This structure causes the "happy path" or intended flow of our code to be indented. I suggest treating the length check like a guard clause to make the main flow of the code more prominent.
if (word.length > 0) {
return 0;
}
score = 0;
for (let letter in word){
score += scoreChart[word[letter].toUpperCase()];
}
if (word.length >=7){
score += 8;
}
return score;|
|
||
| let score = 0; | ||
| if (word.length > 0) { | ||
| for (let letter in 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.
We can iterate over the letters in word directly using a for...of loop:
for (const letter of word) {
score += scoreChart[letter.toUpperCase()];
}If we will not be reassigning the loop variable letter in the body of a loop, I recommend using const over let to declare the variable.
| for (let letter in word){ | ||
| score += scoreChart[word[letter].toUpperCase()]; | ||
| } |
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 could have an unexpected result if the string word has characters that aren't in scoreChart. If we have an input like "ABC DEF" then result will hold NaN (Not a Number) after the loop. If we wanted to skip non-alphabetic characters or characters that aren't in scoreChart, how could we do that?
| highestScoreFrom = (words) => { | ||
| let highestWordAndScore = ["", 0]; | ||
| let score = 0; | ||
| for (let i in 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.
Nice implementation to tie break in a single loop!
No description provided.