-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
} | ||
|
||
|
||
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){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than tracking a separate variable while (drawnLetters.length < HAND_SIZE) { Nit: Leave a space between the |
||
const randomLetter = weightedRandomLetter() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return true | ||
}; | ||
|
||
export const scoreWord = (word) => { | ||
// Implement this method for wave 3 | ||
let score = 0; | ||
|
||
for (let letter of word){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if (score === highestScore) { | ||
possibleWinners.push({word, score}); | ||
} | ||
} | ||
return possibleWinners; | ||
} | ||
|
||
const tieBreaker = possibleWinners => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,7 +62,7 @@ describe("Adagrams", () => { | |
letter_freq[letter] = 1; | ||
} | ||
} | ||
|
||
for (let letter of drawn) { | ||
expect(letter_freq[letter]).toBeLessThanOrEqual(LETTER_POOL[letter]); | ||
} | ||
|
@@ -120,7 +120,9 @@ describe("Adagrams", () => { | |
}); | ||
|
||
it("returns a score of 0 if given an empty input", () => { | ||
throw "Complete test"; | ||
expectScores({ | ||
'': 0 | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", () => { | ||
|
@@ -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") }; | ||
|
@@ -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", () => { | ||
|
There was a problem hiding this comment.
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.