Adagrams-JS-Supriya C #5
Conversation
| "X": 1, | ||
| "Y": 2, | ||
| "Z": 1 | ||
| }; |
There was a problem hiding this comment.
Small style nitpick: I would consider outdenting this closing bracket so it line up with the beginning of line 2.
| let lettersInHand = []; | ||
| while (count < 10){ |
There was a problem hiding this comment.
We could remove the variable count and directly check the length of lettersInHand in the condition.
| const characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; | ||
| let lettersInHand = []; | ||
| while (count < 10){ | ||
| let letterChosen = characters.charAt(Math.floor(Math.random() * 26)); |
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?
There was a problem hiding this comment.
Maybe I will use dictionary of letters to pick random characters. Will do some more research on this.
| for (let i=0; i< input.length; i++){ | ||
| if(dictOfLettersInHand[input[i]] == null){ | ||
| return false; | ||
| } else if (dictOfLettersInHand[input[i]]===0) | ||
| { | ||
| return false; | ||
| } | ||
| dictOfLettersInHand[input[i]] -= 1; | ||
| } |
There was a problem hiding this comment.
Nice approach, I like the use of a frequency map.
To make this a little easier to read and reduce repetition, you could pull dictOfLettersInHand[input[i]] into a variable before the if-statements:
for (let i = 0; i < input.length; i++) {
letterValue = dictOfLettersInHand[input[i]]
if (letterValue == null || letterValue === 0) {
return false;
}
dictOfLettersInHand[input[i]] -= 1;
}| for (let i=0; i< input.length; i++){ | ||
| if(dictOfLettersInHand[input[i]] == null){ |
There was a problem hiding this comment.
Small style issue: we should indent the contents of for-loops so it's clear what code is inside the block.
| if (word.length === 0) | ||
| { |
There was a problem hiding this comment.
Specific teams might have their own conventions, but typically we place the opening brace on the same line as the if-statement.
| let pointsForWord = 0; | ||
| let upperCaseWord = word.toUpperCase(); |
There was a problem hiding this comment.
I suggest placing these variable declarations under the if statement below so they are only created if they will be used.
| for (let i =0; i< upperCaseWord.length; i++){ | ||
| if (upperCaseWord.charAt(i) in lettersDictionary){ | ||
| pointsForWord += lettersDictionary[upperCaseWord.charAt(i)]; | ||
| } | ||
| } |
There was a problem hiding this comment.
We could simplify this loop a little with a for...of loop:
for (const letter of upperCaseWord) {
if (lettersDictionary[letter] != undefined) {
// If its undefined means letter is a special character
pointsForWord += lettersDictionary[letter];
}
}| return 0; | ||
| } | ||
| //Code to add scores of the letters in the word | ||
| for (let i =0; i< upperCaseWord.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.
|
Thank you for your detailed comments. I will remember those and make the changes accordingly. |
2 participants
No description provided.