Skip to content

C22 Phoenix -- Beenish Ali #25

Open
beenishali693 wants to merge 3 commits into
Ada-C22:mainfrom
beenishali693:main
Open

C22 Phoenix -- Beenish Ali #25
beenishali693 wants to merge 3 commits into
Ada-C22:mainfrom
beenishali693:main

Conversation

@beenishali693

Copy link
Copy Markdown

No description provided.

@anselrognlie anselrognlie left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Please review my comments, and let me know if you have any questions.

Comment thread test/adagrams.test.js
throw "Complete test";
expectScores({
'': 0
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon

      });

Comment thread src/adagrams.js
Comment on lines +81 to +84
let totalWeight=0;
for (const weight of WEIGHTS) {
totalWeight += weight;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 Avoid running code in global scope.

Comment thread src/adagrams.js
const letterFreq = {};

while( i < HAND_SIZE){
const randomLetter = weightedRandomLetter()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calculation is an improvement over your Python version (which didn't reflect the weights in how the letters get picked). However, notice that weightedRandomLetter does not adjust to the lower likelihood of picking a particular letter once it has been picked. In the extreme, once all the available copies of a letter have been picked, there should be a 0% chance of picking it going forward. This is not the same thing as excluding the letter if it gets picked again. Rather, its chance of it being picked would need to be distributed throughout the remaining tiles to match the expected behavior of picking tiles from a pile.

This (albeit minor, but still real) discrepency, along with the fact that we still do need to allocate additional memory to ensure that we don't use more copied of a letter than should be available, point towards an approach that truly builds the pile of tiles (a list with as many copies of a letter as there should be tiles) as the method that will most closely provide the probabilities we want, and does so in a predictable number of iterations.

Alternatively, we could decrement the weights of a tiles as they are picked, but this still would require iterating through the letters for each letter, resulting in O(m * n) where m is the size of the hand and n is the number of letters. By contrast, there is a pool-based approach that would be only O(m + n). Of course, for the size of the inputs, these aren't major considerations, but they're still worth considering.

Comment thread src/adagrams.js
let i=0;
const letterFreq = {};

while( i < HAND_SIZE){

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than tracking a separate variable i which we must update independently, we could make this condition dependent on the actual result we're building.

  while (drawnLetters.length < HAND_SIZE) {

Nit: Leave a space between the while and its condition, and between the end of the condition and the block brace.

Comment thread src/adagrams.js
Comment on lines +113 to +116
if (letterFreq[randomLetter] <= LETTER_POOL[randomLetter]){
drawnLetters.push(randomLetter);
i++
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While unlikely, we could end up picking the same letter over and over, to the point that this loop never completes. We would prefer an approach that will predictably pick a letter every iteration.

Comment thread src/adagrams.js
Comment on lines +142 to +144
if (word.length >= MIN_LEN_FOR_BONUS){
score += BONUS_POINTS;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job incorporating named constants instead of using magic numbers. This is much more self-documenting.

Comment thread src/adagrams.js
Comment on lines +155 to +159
if (possibleWinners.length === 1) {
;
} else {
winner = tieBreaker(possibleWinners);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructure this to avoid the empty block. Empty blocks are not a typical pattern we'll see in JS.

  if (possibleWinners.length !== 1) {
    winner = tieBreaker(possibleWinners);
  }

Comment thread src/adagrams.js

const possibleWinners = [];

for (const [word, score] of wordAndScores) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS Maps do preserve the insertion order of keys. In practice, we don't see Maps used that often unless the keys need to be something that isn't a string. Since Maps require the use of the set and get methods, they're a frequent source of bugs (since developers prefer to use []).

Comment thread src/adagrams.js
return winner;
};

const getPossibleWinners = (words) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice helper to get a collection of all the highest score words.

I might restructure into separate steps that

  1. builds a Map of all the words to their scores (removing the need to run scoreWord multiple times for the same word)
  2. Finds the max score
  3. builds a filtered list of the words that have the max score

We could return this as a two value list, the list of words tied, and the score they have. Something like

  [tiedWordList, highestScore]

Elsewhere, I'd break any tie in the filtered word list, which doesn't depend on the score. Then we don't need to build the final return value until the very end of the function.

Comment thread src/adagrams.js
return possibleWinners;
}

const tieBreaker = possibleWinners => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking the ties depends only on the words themselves. Consider restructuring this to receive only a list os words (no score info) and leaving it to the caller to build the necessary retrun value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants