Sphinx - Vlada Rapaport#44
Conversation
| @@ -1,15 +1,163 @@ | |||
| export const drawLetters = () => { | |||
| // Implement this method for wave 1 | |||
| const letterPool = { | |||
There was a problem hiding this comment.
While we use the const keyword to ensure the values cannot be reassigned, we should also use capital letters to convey to other devs the value is fixed.
| const letterPool = { | |
| const LETTER_POOL = { |
| } | ||
|
|
||
| const randomlyHand = []; | ||
| const lettersCountDict = { |
There was a problem hiding this comment.
| const lettersCountDict = { | |
| const LETTERS_COUNT = { |
Recall that Javascript doesn't have dictionaries. It has objects.
| Z: 0, | ||
| }; | ||
|
|
||
| while (randomlyHand.length < 10) { |
There was a problem hiding this comment.
Prefer to reference the integer 10 with a variable. This makes your code more readable/self documenting because it clues other devs in to the meaning of 10.
| while (randomlyHand.length < 10) { | |
| const HAND_LENGTH = 10; | |
| while (randomlyHand.length < HAND_LENGTH) { |
| }; | ||
|
|
||
| while (randomlyHand.length < 10) { | ||
| const i = Math.floor(Math.random() * letters.length); |
There was a problem hiding this comment.
Prefer more descriptive name for this variable, maybe like randomIndex
| let wordLower = input.toLowerCase(); | ||
|
|
||
| for (let letter of lettersInHand) { | ||
| lettersInHandCopy.push(letter.toLowerCase()); | ||
| } | ||
|
|
||
| for (let letter of wordLower) { |
There was a problem hiding this comment.
2 notes:
-
If your looping variable
letteris not updated in the loop, then we should useconstinstead. -
To make your code more concise, call
toLowerCase()when you need it:
| let wordLower = input.toLowerCase(); | |
| for (let letter of lettersInHand) { | |
| lettersInHandCopy.push(letter.toLowerCase()); | |
| } | |
| for (let letter of wordLower) { | |
| for (const letter of lettersInHand.toLowerCase()) { | |
| lettersInHandCopy.push(letter); | |
| } | |
| for (const letter of input.toLowerCase()) { |
| if (!lettersInHandCopy.includes(letter)) { | ||
| return false; | ||
| } else { | ||
| lettersInHandCopy.splice(lettersInHandCopy.indexOf(letter), 1); |
There was a problem hiding this comment.
splice like python's remove has linear time complexity with respect to the length of the list. This won't be a problem for the size of data we're working with, but consider using a strategy of virtually "dividing" the list into a used and unused side. Swapping a value to the used side would be constant time!
|
|
||
| export const scoreWord = (word) => { | ||
| // Implement this method for wave 3 | ||
| const scoreDict = { |
There was a problem hiding this comment.
| const scoreDict = { | |
| const SCORE_DICT = { |
| const bonusLengthMin = 7; | ||
| const bonusLengthMax = 10; | ||
| const bonusPoints = 8; |
There was a problem hiding this comment.
Use all caps for naming thees constant variables
|
|
||
| export const highestScoreFrom = (words) => { | ||
| // Implement this method for wave 4 | ||
| const tieBreakingLength = 10; |
There was a problem hiding this comment.
| const tieBreakingLength = 10; | |
| const TIE_BREAKING_LENGTH = 10; |
| let maxWord = words[0]; | ||
| let maxScore = scoreWord(maxWord); | ||
|
|
||
| for (let i = 1; i < words.length; i++) { |
There was a problem hiding this comment.
Prefer for/of loop instead because we don't need i to access the letter we want to score.
2 participants
No description provided.