Skip to content

WeiQ- Sphinx #29

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
99 changes: 99 additions & 0 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,114 @@
const LETTER_POOL = {

Choose a reason for hiding this comment

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

Nice job using capital letters to name your constant variable!

'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 LetterPoolList = [];
for (const [letter, count] of Object.entries(LETTER_POOL)) {
for (let i = 0; i < count; i++) {
LetterPoolList.push(letter);
}
}
Comment on lines +30 to +35

Choose a reason for hiding this comment

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

Prefer this logic be put in a helper function that you can call in drawLetters

const totalLetterCount = LetterPoolList.length;

Choose a reason for hiding this comment

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

Suggested change
const totalLetterCount = LetterPoolList.length;
const TOTAL_LETTER_COUNT = LetterPoolList.length;



export const drawLetters = () => {
// Implement this method for wave 1
const handOfLetters = [];
for (let i = 0; i < 10; i++) {
const randomIndex = Math.floor(Math.random() * (totalLetterCount - i));
const randonDrawLetter = LetterPoolList[randomIndex];
handOfLetters.push(randonDrawLetter);

const lastIndex = totalLetterCount - i - 1;
[LetterPoolList[randomIndex], LetterPoolList[lastIndex]] = [LetterPoolList[lastIndex], LetterPoolList[randomIndex]]
Comment on lines +47 to +48

Choose a reason for hiding this comment

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

😎 nice work swapping the randomly drawn letter to the end of the list! This a constant time operation which beats an approach that uses splice to remove drawn letters.

}
return handOfLetters;
};


export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
const letterFreMap = {};
for (const letter of lettersInHand) {
if (!(letter in letterFreMap)) {
letterFreMap[letter] = 1;
} else {
letterFreMap[letter] += 1;
}
Comment on lines +56 to +62

Choose a reason for hiding this comment

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

👍 I like the frequency map here so you can leverage the constant time look up.

}
for (const letter of input) {
if (!(letter in letterFreMap) || letterFreMap[letter] < 1) {
return false;
} else {
letterFreMap[letter] -= 1;

Choose a reason for hiding this comment

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

You can decrement like --letterFreMap[letter]

}
}
return true;
};


const SCORE_CHART = {
'A': 1, 'E': 1, 'I': 1, 'O': 1, 'U': 1, 'L': 1, 'N': 1, 'R': 1, 'S': 1, 'T': 1,
'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
}

export const scoreWord = (word) => {
// Implement this method for wave 3
let score = 0
word = word.toUpperCase();
for (let char of word) {
Comment on lines +88 to +89

Choose a reason for hiding this comment

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

We should use const for the looping variable char since this variable's value doesn't change in the loop.

Can also write lines 88-89 in one line:

Suggested change
word = word.toUpperCase();
for (let char of word) {
for (const char of word.toUpperCase()) {

score += SCORE_CHART[char];
}

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

Choose a reason for hiding this comment

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

While the time complexity of using includes in this method ends up being constant because you're always only checking that a word's length is in a list of four integers, I'd prefer this to be written like

Suggested change
if ([7, 8, 9, 10].includes(word.length)) {
if (7 <= word.length <= 10)) {

Writing the check this way feels like it conveys that you're checking the length is between x and y value.

score += 8;
}
return score;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let currentMax = 0;
let currentWinWord = '';
for (const word of words) {
if (scoreWord(word) > currentMax) {
currentMax = scoreWord(word);
currentWinWord = word;
} else if (scoreWord(word) === currentMax && currentWinWord.length != 10) {
Comment on lines +103 to +107

Choose a reason for hiding this comment

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

Between lines 104-107, you invoke the scoreWord method three times. Prefer you calculate the score of the current word once and save it to a variable.

Suggested change
for (const word of words) {
if (scoreWord(word) > currentMax) {
currentMax = scoreWord(word);
currentWinWord = word;
} else if (scoreWord(word) === currentMax && currentWinWord.length != 10) {
for (const word of words) {
const wordScore = scoreWord(word);
if (wordScore > currentMax) {
currentMax = wordScore;
currentWinWord = word;
} else if (wordScore === currentMax && currentWinWord.length != 10) {

if(word.length < currentWinWord.length || word.length === 10)

Choose a reason for hiding this comment

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

Suggested change
if(word.length < currentWinWord.length || word.length === 10)
if (word.length < currentWinWord.length || word.length === 10)

currentWinWord = word;
}
}
return {'word':currentWinWord, 'score':currentMax}
};

8 changes: 5 additions & 3 deletions test/adagrams.test.js
Original file line number Diff line number Diff line change
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.

Suggested change
'':0
'': 0

})
});

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