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

C19 Zoisite | Yvett J. #88

wants to merge 6 commits into from

Conversation

yvett-codes
Copy link

No description provided.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

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

Good work on js-adagrams!

There are several places that you should use const over let. Prefer to use const over let and refactor the variable to use let when you need it.

Let me know if you have questions about my comments!

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

👍

Comment on lines +124 to +126
expectScores({
'': 0,
})

Choose a reason for hiding this comment

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

👍

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

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

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

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

};

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

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

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

Choose a reason for hiding this comment

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

Use const

};

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

Comment on lines +102 to +103
let currentWord = wordSet['word'];
let currentScore = wordSet['score'];

Choose a reason for hiding this comment

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

Use const

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