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.

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

C19 Zoisite | Yvett J. #88

Open
wants to merge 6 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
2 changes: 2 additions & 0 deletions package.json
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
106 changes: 106 additions & 0 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,121 @@
export const drawLetters = () => {
// Implement this method for wave 1
const letters = [

Choose a reason for hiding this comment

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

since letters is a constant variable it should be spelled with all caps like LETTERS

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

Choose a reason for hiding this comment

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

While it was not a requirement for this project to use a dictionary with letters for keys and number of letters for values to create a list of 98 letters, I would prefer to see you do that in order to get practice iterating over a dictionary to create a new data structure.

let hand = [];
let letters2 = [...letters];
Comment on lines +31 to +32

Choose a reason for hiding this comment

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

Use const instead of let since neither variables are reassigned. A general rule of thumb for me when writing javascript is to use const and only refactor to let if the JS interpreter throws an exception when I try to reassign a variable.

Also prefer a more descriptive variable name like lettersCopy

while (hand.length < 10) {
const randomLetter = letters2[Math.floor(Math.random() * letters2.length)];
hand.push(randomLetter);
const index = letters2.indexOf(randomLetter);
letters2.splice(index, 1);
Comment on lines +36 to +37

Choose a reason for hiding this comment

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

The time complexity of indexOf and splice in JS is the same as it is in Python, linear. How could you refactor this method to use an object so that when you look for a letter, you'll have constant time instead of linear?

}
return hand;
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
let inputWord = input.toUpperCase();

Choose a reason for hiding this comment

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

inputWord is never reassigned, should use const instead of let

const inputLetters = inputWord.split('');

const handCounts = {};
for (let letter of lettersInHand) {
handCounts[letter] = handCounts[letter] ? handCounts[letter] + 1 : 1;
}
const inputCounts = {};
for (let letter of inputLetters) {
inputCounts[letter] = inputCounts[letter] ? inputCounts[letter] + 1 : 1;
}

for (let letter in inputCounts) {
if (!(letter in handCounts) || inputCounts[letter] > handCounts[letter]) {
return false;
}
}
return true;

Choose a reason for hiding this comment

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

in the for loops on lines 48, 52, 56 we should use const instead of let because the looping variable letter is not reassigned in the loop.

};

export const scoreWord = (word) => {
// Implement this method for wave 3
const letterScores = {

Choose a reason for hiding this comment

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

Since this is a constant variable, we should name the variable with all caps like LETTER_SCORES

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

let inputWord = word.toUpperCase();

Choose a reason for hiding this comment

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

Use const

const inputLetters = inputWord.split('');
let score = 0;

if (inputWord.length > 6) {
score += 8;
}

for (let letter of inputLetters) {

Choose a reason for hiding this comment

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

Use const instead of let for the looping variable letter since the variable is not reassigned in the loop

score += letterScores[letter];
}
return score;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let winningWord = '';
let topScore = 0;
let wordsWithScores = [];

Choose a reason for hiding this comment

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

wordsWithScores isn't ever reassigned so we should use const instead of let


for (let word of words) {
let score = scoreWord(word);
Comment on lines +96 to +97

Choose a reason for hiding this comment

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

Use const

wordsWithScores.push({'word': word, 'score': score})
}

for (let wordSet of wordsWithScores) {
let currentWord = wordSet['word'];
let currentScore = wordSet['score'];
Comment on lines +102 to +103

Choose a reason for hiding this comment

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

Use const


if (currentScore > topScore) {
topScore = currentScore;
winningWord = currentWord;
} else if (currentScore === topScore) {
if (winningWord.length === 10) {
continue
} else if (currentWord.length === 10) {
topScore = currentScore;
winningWord = currentWord;
} else if (currentWord.length < winningWord.length) {
topScore = currentScore;
winningWord = currentWord;
}
}
}
return {'word': winningWord, 'score': topScore};
};
10 changes: 7 additions & 3 deletions test/adagrams.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ describe("Adagrams", () => {
});

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
// throw "Complete test";
expectScores({
'': 0,
})
Comment on lines +124 to +126

Choose a reason for hiding this comment

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

👍

});

it("adds an extra 8 points if word is 7 or more characters long", () => {
Expand All @@ -133,7 +136,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 +148,8 @@ describe("Adagrams", () => {
const words = ["XXX", "XXXX", "X", "XX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };

throw "Complete test by adding an assertion";
// 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.

👍

});

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