Nadia Sarkissian Zoisite #79
Conversation
src/adagrams.js
Outdated
| @@ -1,15 +1,102 @@ | |||
| export const drawLetters = () => { | |||
| // Implement this method for wave 1 | |||
| }; | |||
| const LETTER_POOL = { | |||
There was a problem hiding this comment.
Since this is a local variable of a function, the best practice is to use camel case for the naming: letterPool
There was a problem hiding this comment.
Ohh, I kept it in all-caps since we treated it like a constant variable the first time. I wasn't confident about doing all-caps here instead of camelCase.
| 'Z': 1 | ||
| }; | ||
|
|
||
| const shuffleArray = array => { |
There was a problem hiding this comment.
Neat use of a nested function!
src/adagrams.js
Outdated
| const playerHand = letterBag.slice(0, 10); | ||
| return playerHand; |
There was a problem hiding this comment.
Since this is a pretty short statement and we don't alter playerHand after creating it, I'd consider directly returning letterBag.slice(0, 10);.
src/adagrams.js
Outdated
|
|
||
| for (let letter of input) { | ||
| const letterIsUnavailable = (letterHashTable[letter] === 0) | ||
| || !(letter in letterHashTable); |
There was a problem hiding this comment.
When dropping a statement across lines, I recommend indenting the continuations one further than the initial line to make it visually clear they are related and the continuations aren't their own new statements.
There was a problem hiding this comment.
I always forget what the best way to do the indentation is! I knew this looked funny.
src/adagrams.js
Outdated
| if (word.length >= 7) { | ||
| score += 8; | ||
| } |
There was a problem hiding this comment.
This could be a great place for a ternary operator:
// Ternary operator structure:
// var = (conditional) ? (value if true) : (value if false)
score = (word.length >= 7) ? (score + 8) : score
src/adagrams.js
Outdated
| if (winningWord.word.length === 10){ | ||
| continue; | ||
| } else if (thisWord.word.length === 10 | ||
| || thisWord.word.length < winningWord.word.length){ | ||
| winningWord = thisWord; | ||
| } | ||
| } |
There was a problem hiding this comment.
Since the result is no change for the first conditional, we could refactor to remove the continue. There are many ways to approach it, one option is to use variables to represent the conditionals we need to consider:
const isBestTen = winningWord.word.length === 10
const isWordTen = thisWord.word.length === 10
const isWordShorter = thisWord.word.length < winningWord.word.length
if ((isWordTen && !isBestTen) || (isWordShorter && !isBestTen)) {
winningWord = thisWord;
}There was a problem hiding this comment.
i love how you set up these complex conditionals! Thank you for this. My spidey sense started tingling ISO a refactor when I had one line that was just "continue"
| score: null | ||
| } | ||
|
|
||
| for (let word of words){ |
There was a problem hiding this comment.
Great tie breaking in a single loop!
There was a problem hiding this comment.
you said the same thing last time! :P
No description provided.