Adagrams-js-Paige-H #81
Conversation
| @@ -1,15 +1,140 @@ | |||
| export const drawLetters = () => { | |||
| // Implement this method for wave 1 | |||
| let lettersDrawn = []; | |||
| const LETTER_POOL = ['A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A', 'B', 'B', 'C', 'C', 'D', 'D', 'D', 'D', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'E', 'F', 'F', 'G', 'G', 'G', 'H', 'H', 'I', 'I', 'I', 'I', 'I', 'I', 'I', 'I', 'I', 'J', 'K', 'L', 'L', 'L', 'L', 'M', 'M', 'N', 'N', 'N', 'N', 'N', 'N', 'O', 'O', 'O', 'O', 'O', 'O', 'O', 'O', 'P', 'P', 'Q', 'R', 'R', 'R', 'R', 'R', 'R', 'S', 'S', 'S', 'S', 'T', 'T', 'T', 'T', 'T', 'T', 'U', 'U', 'U', 'U', 'V', 'V', 'W', 'W', 'X', 'Y', 'Y', 'Z']; | |||
There was a problem hiding this comment.
Nice keeping this close to where it's used! Since LETTER_POOL is a local variable of a function, the best practice is to use camel case for the naming: letterPool
| let letter = LETTER_POOL[Math.floor(Math.random()*LETTER_POOL.length)]; | ||
| let i = LETTER_POOL.indexOf(letter); |
There was a problem hiding this comment.
indexOf is an O(n) operation since it needs to scan through the list for the letter. If we store the random index generated on line 9 in a variable first, we can avoid making the indexOf call:
const index = Math.floor(Math.random()*LETTER_POOL.length)
const letter = LETTER_POOL[index];
if (letter !== undefined){
lettersDrawn.push(letter);
}
delete LETTER_POOL[index];|
|
||
| // if letter.count < letter.count {add letter to lettersDrawn} | ||
| while (lettersDrawn.length < 10) { | ||
| let letter = LETTER_POOL[Math.floor(Math.random()*LETTER_POOL.length)]; |
There was a problem hiding this comment.
If we know we won't be changing the value of a variable, the best practice is to use const to declare it so that we get errors that alert us if something does try to alter it.
| let letter_frequency = []; | ||
| for (let letter of lettersInHand){ | ||
| letter_frequency.push(letter); | ||
| }; |
There was a problem hiding this comment.
When we want to make a copy of an object or list, javascript has a handy operator ... called the spread operator:
const letter_frequency = [...lettersInHand];| 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| score += 8; | ||
| }; | ||
| for (let letter of word.toUpperCase()) { | ||
| switch (letter) { |
There was a problem hiding this comment.
As much as I love a switch statement, I would suggest another approach. This works, but because of the number of cases, a reader likely needs to scroll to see the whole structure and track the fallthrough chain to get an idea of what the value for a particular letter is.
| if (length_of_first_word === 10) { | ||
| return words_and_scores[first_index]; | ||
| } else { | ||
| return words_and_scores[second_index]; | ||
| }; |
There was a problem hiding this comment.
This solution only considers that there could be 2 possible words we need to choose from, however, our function needs to be able to tie break between an arbitrarily large number of elements. It's valid and possible that we might have 3 or 5 or more words tied for highest score. I would like to see you revisit this function to create a more general solution. It should also pass additional tests that have 3 or more words for input, the following tests I added to the "in case of tied score" describe block for the "highestScoreFrom" tests and should pass.
it("selects the word with fewer letters when neither are 10 letters", () => {
const words = ["MMMM", "WWW", "BBBB"];
const correct = { word: "WWW", score: scoreWord("WWW") };
expectTie(words);
expect(highestScoreFrom(words)).toEqual(correct);
expect(highestScoreFrom(words.reverse())).toEqual(correct);
});
it("selects the first 10-letter word when all 3 words have the same score", () => {
const words = ["AAAAAAAAAA", "BBBBBB", "EEEEEEEEEE"];
const first = {
word: "AAAAAAAAAA",
score: scoreWord("AAAAAAAAAA"),
};
const second = {
word: "EEEEEEEEEE",
score: scoreWord("EEEEEEEEEE"),
};
expectTie(words);
expect(highestScoreFrom(words)).toEqual(first);
expect(highestScoreFrom(words.reverse())).toEqual(second);
});
No description provided.