Skip to content

ivy s - lions#128

Open
ivystrayed wants to merge 5 commits intoAda-C18:mainfrom
ivystrayed:main
Open

ivy s - lions#128
ivystrayed wants to merge 5 commits intoAda-C18:mainfrom
ivystrayed:main

Conversation

@ivystrayed
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. For commits, it would be great if the commit messages could be more descriptive. Instead of talking about which wave was completed, something like "Created drawLetters function" would be great. Great job!

Comment thread src/adagrams.js
// Implement this method for wave 1
};

const letterPool = {
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 that it doesn't have to be recreated each time the function is called.

Comment thread src/adagrams.js
Comment on lines +36 to +52
let letterList = [];

for (let letter in letterPool){
let freq = letterPool[letter];
for (let i = 0 ; i < freq ; i++) {
letterList.push(letter);
}
}

let hand = [];
while (hand.length < 10){
let index = Math.floor(Math.random()*letterList.length);
let letterDrawn = letterList[index];
hand.push(letterDrawn);
letterList.splice(index, 1);
}
return hand;
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

}

export const usesAvailableLetters = (input, lettersInHand) => {
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
export const scoreWord = (word) => {
// Implement this method for wave 3
};
const pointValues = {
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 +100 to +111
let total = 0;
let wordUpper = word.toUpperCase();

if (6 < word.length && word.length < 11) {
total += 8
}
for (let letter of wordUpper){
let letPoint = pointValues[letter];
total += letPoint;
}

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

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({

})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like this test was not completed. Make sure to complete all tests!

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