Skip to content
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.

Sign up for GitHub

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

Kunzite--Tuminello, Ann #54

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Kunzite--Tuminello, Ann #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1da17b8
push yarn and eslint to github
AKTuminello May 31, 2023
55f5e80
Wave I commit, some tests working
AKTuminello May 31, 2023
2a015df
some work on usesAvailableLetters
AKTuminello Jun 1, 2023
ad921e5
Wave three completed, zero score test written
AKTuminello Jun 2, 2023
8a3c046
Complete Wave 4 of Adagrams, Tests passing
AKTuminello Jun 4, 2023
0822e56
Cleanup
AKTuminello Jun 4, 2023
ccee07f
working on demo tests. Unsure.
AKTuminello Jun 5, 2023
208d702
model testing not implmented
AKTuminello Jun 5, 2023
3d44919
all model tests working
AKTuminello Jun 7, 2023
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
5 changes: 5 additions & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"cSpell.words": [
"adagrams"
]
}
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
"babel-core": "^7.0.0-bridge.0",
"babel-jest": "^24.8.0",
"babel-plugin-module-resolver": "^3.2.0",
"eslint": "^8.41.0",
"eslint-plugin-jest": "^27.2.1",
"jest": "^24.8.0",
"regenerator-runtime": "^0.12.1"
},
Expand Down
163 changes: 154 additions & 9 deletions src/adagrams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,160 @@
export const drawLetters = () => {
// Implement this method for wave 1
const letterPool = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider defining letterPool outside the function to focus on the code. Since the logic modifies the letter counts, we would need to make a copy of the pool data so as not to clobber the data for subsequent calls.

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

let hand = [];

for (let i = 0; i < 10; i++) {
let letter = Object.keys(letterPool)[Math.floor(Math.random() * 26)];
Copy link
Contributor

Choose a reason for hiding this comment

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

👀Object.keys must iterate over the data each time it's called. Prefer performing this call once outside the loop, and use its result each time through the loop to significantly reduce the iterations required.

This logic selects letters with equal weighting. You have logic to reject invalid letters (letters we've already used up), but be aware that this will tend to select hands with an overrepresentation of "hard" letters, which would make it harder for the player to form words.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀 Prefer to avoid "magic numbers" like 26. Instead, stored the result of Object.keys in a variable, and then we could use its length!

if (letterPool[letter] > 0) {
hand.push(letter);
letterPool[letter] -= 1;
} else {
i -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 If our for loop requires manually manipulating the index variable, then maybe a while loop is a better match for the logic. While we can do this, it's usually confusing and easy to overlook.

The driver for this here is that the main loop might not successfully pick a letter on each loop, meaning we don't know how many times the loop should run. We can capture that by rewriting as a loop that runs while the hand length is less than the needed number of letters.

}
}

return hand;
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
};

export const scoreWord = (word) => {
// Implement this method for wave 3
};
const letterBankLower = lettersInHand.map((letter) => letter.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice use of map to apply toLowerCase (a string method) to the individual lettes in the array. Unfortunatrely for JS, there's a bit more of a split between strings and arrays that we see in Python. This also makes a copy, which is important since you'll be modifying this list.

const inputLower = input.toLowerCase();

for (const letter of inputLower) {
const index = letterBankLower.indexOf(letter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that indexOf is linear time on the length of the array. So we're needing to loop over the hand for each letter in the word. Consider using a frequency map to tally up the counts of all the hand letters in a single pass.

if (index !== -1) {
letterBankLower.splice(index, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does account for consuming letters, but splice is also a linear time operation. Using a frequency map would allow checking and updating the remaining hand in constant time per letter instead.

} else {
return false;
}
Comment on lines +53 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 Consider inverting this into a guard clause

      if (index === -1) {
        return false;
      }

      letterBankLower.splice(index, 1);

}
return true;
};

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

export const scoreWord = (word) => {
const scoreChart = {
'A': 1, 'E': 1, 'I': 1, 'O': 1, 'U': 1, 'L': 1, 'N': 1, 'R': 1, 'S': 1, 'T': 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 Prefer to list this out alphabetically. It's hard for a human reader to be able to tell whether all the letters are there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving the score chart out of the function to put the focus on the logic.

'D': 2, 'G': 2,
'B': 3, 'C': 3, 'M': 3, 'P': 3,
'F': 4, 'H': 4, 'V': 4, 'W': 4, 'Y': 4,
'K': 5,
'J': 8, 'X': 8,
'Q': 10, 'Z': 10
};

if (word === ''){
return 0;
}

Comment on lines +74 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do without this special case.

let score = 0;
const wordUpper = word.toUpperCase();

for (let i = 0; i < wordUpper.length; i++) {
const letter = wordUpper[i];
score += scoreChart[letter] || 0;
}
Comment on lines +81 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 Prefer for/of

    for (const letter of wordUpper) {
      score += scoreChart[letter] || 0;
    }


if ([7, 8, 9, 10].includes(word.length)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider sticking with relational operators (> <) rather than using list membership

score += 8;
}

return score;
};


export const highestScoreFrom = (words) => {
if (words.length === 0) {
return null;
}

let highestScore = 0;
let winningWord = null;

for (const word of words) {
const wordScore = scoreWord(word);

if (wordScore > highestScore) {
highestScore = wordScore;
winningWord = word;
} else if (wordScore === highestScore) {
if (word.length === 10 && winningWord.length !== 10) {
winningWord = word;
} else if (word.length < winningWord.length && winningWord.length !== 10) {
winningWord = word;
}
Comment on lines +105 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice capturing of all the cases we need to handle ties. For a more javascript-esque approach, rather than a plain for loop, consider using reduce!

}
}

return { word: winningWord, score: highestScore };
};



/* The original Pseudocode for Wave 4--The Python version
PSEUDOCODE for WAVE 4:

FUNCTION get_highest_word_score(word_list):
highest_score = 0
winning_word = ""
FOR word IN word_list:
word_score = score_word(word)
IF word_score > highest_score:
highest_score = word_score
winning_word = word
ELSE IF word_score == highest_score:
IF LENGTH OF word == 10 AND LENGTH OF winning_word != 10:
winning_word = word
ELSE IF LENGTH OF word < LENGTH OF winning_word AND LENGTH OF winning_word != 10:
winning_word = word
RETURN A TUPLE CONTAINING winning_word AND highest_score

Roughly translated to JS:
export const highestScoreFrom = (words) => {
if words is empty:
return null

highestScore = 0
winningWord = null

for each word in words:
wordScore = scoreWord(word)

if wordScore > highestScore or (wordScore === highestScore and winningWord is null):
highestScore = wordScore
winningWord = word
else if wordScore === highestScore:
if word.length === 10 and winningWord.length !== 10:
winningWord = word
else if word.length < winningWord.length and winningWord.length !== 10:
winningWord = word

return { word: winningWord, score: highestScore }*/
90 changes: 56 additions & 34 deletions test/adagrams.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,19 @@ describe("Adagrams", () => {
});
});

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
it('returns a score of 0 if given an empty input', () => {
// Arrange
const word = '';

// Act
const result = scoreWord(word);

// Assert
expect(result).toBe(0);
Comment on lines +127 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Note that we could have used the same helper as the other score tests.

      expectScores({
        "": 0,
      });

});


it("adds an extra 8 points if word is 7 or more characters long", () => {
it('adds an extra 8 points if word is 7 or more characters long', () => {
expectScores({
XXXXXXX: 64,
XXXXXXXX: 72,
Expand All @@ -133,61 +141,74 @@ describe("Adagrams", () => {
});
});

describe.skip("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") };

describe('highestScoreFrom', () => {
it('returns a hash that contains the word and score of the best word in an array', () => {
const words = ['X', 'XX', 'XXX', 'XXXX'];
const correct = { word: 'XXXX', score: scoreWord('XXXX') };
expect(highestScoreFrom(words)).toEqual(correct);
});

it("accurately finds best scoring word even if not sorted", () => {
const words = ["XXX", "XXXX", "X", "XX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };

throw "Complete test by adding an assertion";
it('accurately finds the best scoring word even if not sorted', () => {
const words = ['XXX', 'XXXX', 'X', 'XX'];
const correct = { word: 'XXXX', score: scoreWord('XXXX') };
expect(highestScoreFrom(words)).toEqual(correct);
});

describe("in case of tied score", () => {

it('returns null for an empty array', () => {
const words = [];

expect(highestScoreFrom(words)).toBeNull();
});

it('returns the only word in the array if it is the highest-scoring word', () => {
const words = ['APPLE'];
const correct = { word: 'APPLE', score: scoreWord('APPLE') };

expect(highestScoreFrom(words)).toEqual(correct);
});
Comment on lines +159 to +170
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice custom tests. The specification didn't describe the empty list behavior, but returning null is one possibility. We might consider throwing an error, though JS is a little less error-happy than is Python.

});
describe('in case of tied score', () => {
const expectTie = (words) => {
const scores = words.map((word) => scoreWord(word));
const highScore = scores.reduce((h, s) => (h < s ? s : h), 0);
const tiedWords = scores.filter((s) => s == highScore);
const scores = words.map((word) => scoreWord(word));
const highScore = scores.reduce((h, s) => (h < s ? s : h), 0);
const tiedWords = scores.filter((s) => s == highScore);

// Expect at least two tied words
expect(tiedWords.length).toBeGreaterThan(1);
expect(tiedWords.length).toBeGreaterThan(1);
};

it("selects the word with 10 letters", () => {
const words = ["AAAAAAAAAA", "BBBBBB"];
it('selects the word with 10 letters', () => {
const words = ['AAAAAAAAAA', 'BBBBBB'];
const correct = {
word: "AAAAAAAAAA",
score: scoreWord("AAAAAAAAAA"),
word: 'AAAAAAAAAA',
score: scoreWord('AAAAAAAAAA'),
};
expectTie(words);

expect(highestScoreFrom(words)).toEqual(correct);
expect(highestScoreFrom(words.reverse())).toEqual(correct);
});

it("selects the word with fewer letters when neither are 10 letters", () => {
const words = ["MMMM", "WWW"];
const correct = { word: "WWW", score: scoreWord("WWW") };
it('selects the word with fewer letters when neither are 10 letters', () => {
const words = ['MMMM', 'WWW'];
const correct = { word: 'WWW', score: scoreWord('WWW') };
expectTie(words);

expect(highestScoreFrom(words)).toEqual(correct);
expect(highestScoreFrom(words.reverse())).toEqual(correct);
});

it("selects the first word when both have same length", () => {
const words = ["AAAAAAAAAA", "EEEEEEEEEE"];
it('selects the first word when both have same length', () => {
const words = ['AAAAAAAAAA', 'EEEEEEEEEE'];
const first = {
word: "AAAAAAAAAA",
score: scoreWord("AAAAAAAAAA"),
word: 'AAAAAAAAAA',
score: scoreWord('AAAAAAAAAA'),
};
const second = {
word: "EEEEEEEEEE",
score: scoreWord("EEEEEEEEEE"),
word: 'EEEEEEEEEE',
score: scoreWord('EEEEEEEEEE'),
};
expectTie(words);

Expand All @@ -196,4 +217,5 @@ describe("Adagrams", () => {
});
});
});
});


2 changes: 1 addition & 1 deletion test/demo/model.test.js
View file
Open in desktop
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
Loading