Skip to content

C22 Phoenix -- Beenish Ali #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 184 additions & 4 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,195 @@
const LETTER_POOL = {
'A': 9,
'B': 2,
'C': 2,
'D': 4,
'E': 12,
'F': 2,
'G': 3,
'H': 2,
'I': 9,
'J': 1,
'K': 1,
'L': 4,
'M': 2,
'N': 6,
'O': 8,
'P': 2,
'Q': 1,
'R': 6,
'S': 4,
'T': 6,
'U': 4,
'V': 2,
'W': 2,
'X': 1,
'Y': 2,
'Z': 1
};
const SCORE_CHART = {
'A': 1,
'B': 3,
'C': 3,
'D': 2,
'E': 1,
'F': 4,
'G': 2,
'H': 4,
'I': 1,
'J': 8,
'K': 5,
'L': 1,
'M': 3,
'N': 1,
'O': 1,
'P': 3,
'Q': 10,
'R': 1,
'S': 1,
'T': 1,
'U': 1,
'V': 4,
'W': 4,
'X': 8,
'Y': 4,
'Z': 10
}


const LETTERS = [
'A', 'B', 'C', 'D', 'E', 'F',
'G', 'H', 'I', 'J', 'K', 'L',
'M', 'N', 'O', 'P', 'Q', 'R',
'S', 'T', 'U', 'V', 'W', 'X',
'Y', 'Z'];

const WEIGHTS = [
9, 2, 2, 4, 12, 2, 3, 2,
9, 1, 1, 4, 2, 6, 8, 2,
1, 6, 4, 6, 4, 2, 2, 1,
2, 1
];

const HAND_SIZE = 10;

const MIN_LEN_FOR_BONUS = 7;

const BONUS_POINTS = 8;

const TIE_BREAKER_WORD_LEN = 10;

let totalWeight=0;
for (const weight of WEIGHTS) {
totalWeight += weight;
}
Comment on lines +81 to +84

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.



const weightedRandomLetter = () => {

const randomNum = Math.random() * totalWeight;

let currentWeight = 0;
for (let i = 0; i < LETTERS.length; i++) {
currentWeight += WEIGHTS[i];
if (currentWeight >= randomNum) {
return LETTERS[i]
}
}
};


export const drawLetters = () => {
// Implement this method for wave 1
const drawnLetters = []
let i=0;
const letterFreq = {};

while( i < HAND_SIZE){

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.

const randomLetter = weightedRandomLetter()

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.

if (randomLetter in letterFreq) {
letterFreq[randomLetter] += 1;
} else {
letterFreq[randomLetter] = 1;
}
if (letterFreq[randomLetter] <= LETTER_POOL[randomLetter]){
drawnLetters.push(randomLetter);
i++
}
Comment on lines +113 to +116

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.

}
return drawnLetters
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
const lettersInHandCopy = Array.from(lettersInHand);
for (const letter of input){
if (lettersInHandCopy.includes(letter) === false){
return false;
} else {
const index = lettersInHandCopy.indexOf(letter);
lettersInHandCopy.splice(index, 1);
}
}
Comment on lines +124 to +130

Choose a reason for hiding this comment

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

Watch the indentation. Though it doesn't control the logic flow, as it does in Python, it's still easier for others to read our code if we maintain consistent indentation.

Choose a reason for hiding this comment

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

includes, indexOf, and splice are all linear with respect to the size of the hand, and are being called in an outer loop, resulting in O(m * n) time complexity, where m is the length of the word, and n is the size of the hand. While this is unlikely to be a problem for the size of our inputs, we should still consider this for our overall code approach. We would probably prefer a frequency map approach here, which would reduce the time complexity to O(m + n).

return true
};

export const scoreWord = (word) => {
// Implement this method for wave 3
let score = 0;

for (let letter of word){

Choose a reason for hiding this comment

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

Prefer const

  for (const letter of word){

letter = letter.toUpperCase()
score += SCORE_CHART[letter];
}

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

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.


return score;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4

const possibleWinners = getPossibleWinners(words);

let winner = possibleWinners[0];

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

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);
  }

return winner;
};

const getPossibleWinners = (words) => {

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.

const wordAndScores = new Map();

for (const word of words){
wordAndScores.set(word, scoreWord(word))
}

const highestScore = Math.max(...wordAndScores.values());

const possibleWinners = [];

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

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 []).

if (score === highestScore) {
possibleWinners.push({word, score});
}
}
return possibleWinners;
}

const tieBreaker = possibleWinners => {

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.

let fewestLetters = possibleWinners[0].word.length;
let winner = possibleWinners[0];

for (const possibleWinner of possibleWinners){
if (possibleWinner.word.length === TIE_BREAKER_WORD_LEN){
return possibleWinner;
} else if (possibleWinner.word.length < fewestLetters){
fewestLetters = possibleWinner.word.length;
winner = possibleWinner;
}
}
return winner;
}
10 changes: 6 additions & 4 deletions test/adagrams.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ describe("Adagrams", () => {
letter_freq[letter] = 1;
}
}

for (let letter of drawn) {
expect(letter_freq[letter]).toBeLessThanOrEqual(LETTER_POOL[letter]);
}
Expand Down Expand Up @@ -120,7 +120,9 @@ describe("Adagrams", () => {
});

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
expectScores({
'': 0
})

Choose a reason for hiding this comment

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

Missing semicolon

      });

});

it("adds an extra 8 points if word is 7 or more characters long", () => {
Expand All @@ -133,7 +135,7 @@ describe("Adagrams", () => {
});
});

describe.skip("highestScoreFrom", () => {
describe("highestScoreFrom", () => {
it("returns a hash that contains the word and score of best word in an array", () => {
const words = ["X", "XX", "XXX", "XXXX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };
Expand All @@ -145,7 +147,7 @@ describe("Adagrams", () => {
const words = ["XXX", "XXXX", "X", "XX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };

throw "Complete test by adding an assertion";
expect(highestScoreFrom(words)).toEqual(correct);
});

describe("in case of tied score", () => {
Expand Down
2 changes: 1 addition & 1 deletion test/demo/model.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Model from 'demo/model';
import Adagrams from 'demo/adagrams';

describe.skip('Game Model', () => {
describe ('Game Model', () => {
const config = {
players: [
'Player A',
Expand Down