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

Lions - Suzanne S18 #42

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

Lions - Suzanne S18 #42

wants to merge 9 commits into from

Conversation

Suzponeill
Copy link

No description provided.

Copy link

@nancy-harris nancy-harris left a comment

Choose a reason for hiding this comment

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

Excellent job! There are some comments below on places to make the code simpler/cleaner. There is also a comment on a function that does not work the way it is supposed to.

@@ -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.

Excellent!

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.

Excellent!

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

Choose a reason for hiding this comment

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

It would be good to have this declared outside of the function so it doesn't have to be recreated each time the function is called.

Y: 2,
Z: 1,
};
function getRandomLetter(letterBank) {

Choose a reason for hiding this comment

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

It would be good to have this declared outside of the function so it doesn't have to be recreated each time the function is called.

Comment on lines +38 to +47
const hand = [];
while (hand.length < 10) {
const randomLetter = getRandomLetter(letterBank);
const occurance = hand.filter((x) => x === randomLetter).length;
if (occurance < letterBank[randomLetter]) {
hand.push(randomLetter);
}
}

return hand;

Choose a reason for hiding this comment

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

Excellent!

for (let inputLetter of input) {
const index = drawn.indexOf(inputLetter);
if (index > -1) {
drawn.splice(index, 1);

Choose a reason for hiding this comment

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

We should not be removing letters from the drawn array. All this function should do is check to see if all of the letters in input are in drawn. If letters are being removed every time we're just checking to see if the input word is an anagram, we'll run out of letters pretty quickly.

};

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

Choose a reason for hiding this comment

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

It would be good to declare this outside of the function.

Comment on lines +74 to +80
for (let letter of word.toUpperCase()) {
let key = Object.keys(scoreChart).filter(function (key) {
return key.includes(letter);
});
const score = scoreChart[key];
total += score;
}

Choose a reason for hiding this comment

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

It would be more efficient to store the scores with one letter to one number value. Looking up a single key in a JS object is O(1) time complexity. Here, you need to look through every letter to find the one that matches.

Comment on lines +110 to +112
{
break;
}

Choose a reason for hiding this comment

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

Since we know this is definitely the best word, we can just do return wordObj from here and avoid the logic at the bottom looking for the object in the array again.

break;
}
} else if (wordObj.word.length < minLength) {
bestWord = wordObj.word;

Choose a reason for hiding this comment

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

Instead of keeping track of just the best word, we can keep track of the whole wordObj that's the current best. This will allow us to avoid the logic later of trying to find the object again.

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