Skip to content

Nancy L - Lions#123

Open
nancy-lee89 wants to merge 11 commits intoAda-C18:mainfrom
nancy-lee89:main
Open

Nancy L - Lions#123
nancy-lee89 wants to merge 11 commits intoAda-C18:mainfrom
nancy-lee89:main

Conversation

@nancy-lee89
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@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 cleaner/simpler. There is also a function that does not work as expected. See comments below. Great job!

Comment thread src/adagrams.js
// const drawLetters = () => {
// Implement this method for wave 1

const availableLetters = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This can be declared outside of the function so it doesn't have to be recreated each time the function is called.

Comment thread src/adagrams.js
Comment on lines +35 to +51
const letterBowl = [];

Object.keys(availableLetters).forEach((key) => {
const quantity = availableLetters[key];
for (let i = 0; i < quantity; i++) {
letterBowl.push(key);
}
});

const letterHand = [];
for (let i = 0; i < 10; i++) {
const i = Math.floor(Math.random() * letterBowl.length);
const randomLetter = letterBowl[i];
letterHand.push(randomLetter);
letterBowl.splice(i, 1);
}
return letterHand;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

Comment thread src/adagrams.js
if (lettersInHand.includes(letter)) {
// remove letter from lettersInHand
const letterIndex = lettersInHand.indexOf(letter);
lettersInHand.splice(letterIndex, 1);
Copy link
Copy Markdown

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 lettersInHand. This function should only check if all of the letters in input are in lettersInHand. If we're removing letters each time we're doing this check, the user will run out of letters quickly.

Comment thread src/adagrams.js
const letterIndex = lettersInHand.indexOf(letter);
lettersInHand.splice(letterIndex, 1);
} else {
submittedLetter = false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of having a variable to track this, we can just return false as soon as we hit this condition even once. We'll also save ourselves from needing to check the rest of input.

Comment thread src/adagrams.js

export const scoreWord = (word) => {
// Implement this method for wave 3
const letterValues = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This can be declared outside of the function.

Comment thread src/adagrams.js
Comment on lines +103 to +114
const capitalizedWord = word.toUpperCase();

let score = 0;

for (let letter = 0; letter < capitalizedWord.length; letter++) {
let wordValue = capitalizedWord[letter];
score += letterValues[wordValue];
}
if (capitalizedWord.length >= 7) {
score += 8;
}
return score;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

Comment thread src/adagrams.js
return score;
};

export const highestScoreFrom = (words) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

Comment thread test/adagrams.test.js
Comment on lines +123 to +125
expectScores({
"": 0,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

Comment thread test/adagrams.test.js
const correct = { word: "XXXX", score: scoreWord("XXXX") };

throw "Complete test by adding an assertion";
expect(highestScoreFrom(words)).toEqual(correct);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

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