AC2-Ocelots-Elaine(Si Ok) Sohn - #19
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Looks good! I’ve added some suggestions, let me know if there's anything I can clarify ^_^
| export const usesAvailableLetters = (input, lettersInHand) => { | ||
| // Implement this method for wave 2 | ||
| }; | ||
| LETTER_SCORE = { |
There was a problem hiding this comment.
Since these constants at the top of the file are only accessed by one function each, it would be reasonable to place the objects inside the functions rather than as a constant. There are tradeoffs, the structure would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean that other functions who shouldn't need it couldn't access it.
| // Implement this method for wave 1 | ||
| }; | ||
| class Adagrams{ | ||
| LETTER_POOL = { |
There was a problem hiding this comment.
If these shouldn't be edited, I would consider creating them as a static, read-only properties: https://stackoverflow.com/a/35242218
|
|
||
| drawLetters() { | ||
| while (this.lettersInHand.length < 10) { | ||
| let letter = String.fromCharCode(Math.floor(Math.random() * 26) + 65); |
There was a problem hiding this comment.
This will give us an even chance of picking any single letter in the alphabet without going over the number of each tile we have. This is slightly different than what the README asks - we won't accurately represent the distribution of tiles because we pick a letter from 1-26, when the chances of picking some letters should be higher than others. How could we update the algorithm to account for this?
| }; | ||
|
|
||
| drawLetters() { | ||
| while (this.lettersInHand.length < 10) { |
There was a problem hiding this comment.
Nice use of the hand length to control the while loop
| if (this.LETTER_POOL[letter] > 0) { | ||
| this.LETTER_POOL[letter] -= 1; | ||
| this.lettersInHand.push(letter); | ||
| } |
There was a problem hiding this comment.
If we are directly editing this.LETTER_POOL, what will happen after we call drawLetters on the same instance many times in a row?
| }; | ||
|
|
||
| scoreWord(word) { | ||
| this.word = word.toUpperCase(); |
There was a problem hiding this comment.
We wouldn't typically hold onto the input of a function like this as an attribute on a class. If it isn't important to persist between function calls, then we should re-evaluate if it's something the class itself should hold.
| let point = 0; | ||
|
|
||
| for (let i=0; i < this.word.length; ++i){ | ||
| point += this.LETTER_SCORE[this.word[i]]; | ||
| } |
There was a problem hiding this comment.
We could simplify the syntax here a little with a for...of loop:
for (const letter of this.word) {
point += this.LETTER_SCORE[letter];
}| } | ||
| let point = 0; | ||
|
|
||
| for (let i=0; i < this.word.length; ++i){ |
There was a problem hiding this comment.
If we won't be reassigning the loop variable in the body of a loop, I recommend using const over let to declare the variable.
| let point = 0; | ||
|
|
||
| for (let i=0; i < this.word.length; ++i){ | ||
| point += this.LETTER_SCORE[this.word[i]]; | ||
| } |
There was a problem hiding this comment.
This could have an unexpected result if the string word has characters that aren't in LETTER_SCORE . If we have an input like "ABC DEF" then point will hold NaN (Not a Number) after the loop. If we wanted to skip non-alphabetic characters or characters that aren't in LETTER_SCORE , how could we do that?
|
|
||
| highestScoreFrom(words) { | ||
|
|
||
| for (let word of words) { |
There was a problem hiding this comment.
Nice solution to tie break in a single loop!
2 participants
No description provided.