Skip to content

Sphinx - Brianna R.#32

Open
bri-root wants to merge 4 commits into
Ada-C22:mainfrom
bri-root:main
Open

Sphinx - Brianna R.#32
bri-root wants to merge 4 commits into
Ada-C22:mainfrom
bri-root:main

Conversation

@bri-root

Copy link
Copy Markdown

No description provided.

@mikellewade mikellewade left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work! Please let me know if there are any questions about the comments made.

Comment thread src/adagrams.js
Comment on lines +45 to +49
for (const [key, value] of Object.entries(letterPool)) {
for (let i = 0; i < value; i++) {
letterFrequency.push(key)
};
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work finding the javascript version of .items!

Comment thread src/adagrams.js
Comment on lines +53 to +59
do{
const position = Math.floor(Math.random() * (letterFrequency.length-1));
const randomLetter = letterFrequency[position];
hand.push(randomLetter);
letterFrequency.splice(position, 1);
i++;
} while (i <= 10);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there a particular reason as to why you implemented a do...while loop rather than a while loop? The only difference is that the do...while loop is guaranteed to run at least once, but this isn't necessarily being leveraged here. (If you were just testing things out feel free to ignore this comment.)

Comment thread src/adagrams.js
i++;
} while (i <= 10);

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.

Something extra to think about is how you could implement this same functionality with the ... syntax that is provided and if you think it provides any advantages against what you have.

Comment thread src/adagrams.js
const letterList = []
const letterBankCopy = lettersInHand.slice()

for (let letter of word) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice, you used the appropriate for loop!

Comment thread src/adagrams.js
for (let letter of word) {
if (letterBankCopy.includes(letter)) {
letterList.push(letter);
letterBankCopy.splice(letter, 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.

These are inputs are going to be the same size (10) but what if they weren't? What if the input size varied? What would be the time complexity of your code? What CS fundamentals data structure did we learn about that is handy for keeping track of the number of occurrences?

Comment thread src/adagrams.js
Comment on lines +79 to +90
const upperCaseWord = word.toUpperCase();
let score = 0;

for (let letter of upperCaseWord) {
score = score + scoreChart[letter];
};

if (word.length > 6) {
score = score + 8;
};
console.log(score);
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.

Nice work, very legible function! Don't forget to remove your console.logs!

Comment thread src/adagrams.js
Comment on lines +102 to +103
} else if (score < bestScore) {
null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this line serving some purpose I don't see? Right now it seems like it isn't actually contributing to your logic.

Comment thread src/adagrams.js
};
let result = {score: bestScore, word : bestWord};
return result;
}; No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great work on making this logic easy to follow!

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.

3 participants