Skip to content

Pheonix class: Madeline Bennett #31

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 4 commits into
base: main
Choose a base branch
from
Open

Conversation

MBsea21
Copy link

@MBsea21 MBsea21 commented Nov 27, 2024

No description provided.

…sts: "draws ten letters from pool ", "returns an array, and each is a single letter str", "does not draw a letter too many times".
…d helper function that turns arrays into an occurence dictionary. all tests pass from part one and part two.
Copy link

@anselrognlie anselrognlie left a comment

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.

"version": "0.2.0",
"resolved": "https://registry.npmjs.org/@eslint/plugin-kit/-/plugin-kit-0.2.0.tgz",
"integrity": "sha512-vH9PiIMMwvhCx31Af3HiGzsVNULDbyVkHXwlemn/B0TFj/00ho3y55efXrUZTfQipxoHC5u4xq6zblww1zm1Ig==",
"version": "0.2.3",

Choose a reason for hiding this comment

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

Any idea what caused these changes in the lock file? Not a problem that they were, but with a typical project setup, this file wouldn't have been modified.

Comment on lines +123 to +125
expectScores({
'': 0
});

Choose a reason for hiding this comment

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

👍

@@ -133,7 +135,7 @@ describe("Adagrams", () => {
});
});

describe.skip("highestScoreFrom", () => {
describe("highestScoreFrom", () => {

Choose a reason for hiding this comment

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

👍

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

Choose a reason for hiding this comment

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

👍

@@ -1,15 +1,166 @@
export const drawLetters = () => {
// Implement this method for wave 1
const letterPool = {

Choose a reason for hiding this comment

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

Consider moving the dictionary of letters to be a global constant. This is pretty common to do with large data values, so that the body of the function isn't dominated with just the data, which we have to scroll past to find the actual logic. Also, while it would only be a small cost, JS does recreate the dictionary over and over each time drawLetters is called, whereas if we move it outside the function, it will be initialized only once.


//// helper functions///////

const arrayToOccurenceDict = (array) => {

Choose a reason for hiding this comment

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

Nice helper to build a frequency map from something iterable.

return occurenceDictionary
}

const getRandomInt = (min, max) => {

Choose a reason for hiding this comment

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

👀 Because the previous closing brace was improperly indented, that's thrown off the formatting of everything that follows.

Comment on lines +155 to +157
//6 points:
'J' : 8, 'X' : 8,
//7 points:

Choose a reason for hiding this comment

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

Lying comments 😭

Comment on lines +145 to +158
//1 point
'A' : 1, 'E' : 1, 'I' : 1, 'O' : 1 , 'U' : 1, 'L' : 1, 'N' : 1, 'R' : 1, 'S' : 1, 'T' : 1,
//2 points
'D' : 2, 'G' : 2,
//3 points
'B' : 3, 'C' : 3, 'M' : 3, 'P' : 3,
//4 points:
'F' : 4, 'H' : 4, 'V' : 4, 'W' : 4, 'Y': 4,
//5 points:
'K' : 5,
//6 points:
'J' : 8, 'X' : 8,
//7 points:
'Q' : 10, 'Z' : 10

Choose a reason for hiding this comment

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

Since JS won't care what order the letters are listed, prefer listing them in a way that's easier for humans to check that everything is present and accounted for. Writing the letters alphabetically would make it easier for the reader to confirm that all letters are listed with no gaps.

Comment on lines +160 to +162
if (isNaN(lettersAndValues[key])){
return letterOccurenceDict[key]
}

Choose a reason for hiding this comment

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

This is puzzling to me. My interpretation of the intent is that if the specified letter doesn't appear in the score data, that it will use the number of occurrences as the score. Why would an "unscored" letter be worth any score?

We didn't have any test cases where you would receive an unscored letter, but if I were in that situation, I would either ignore the invalid letter (give it a 0 score), or raise some sort of error.

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