Skip to content

Salma Anany - C22- Sphinx#21

Open
SalmaAnany wants to merge 1 commit into
Ada-C22:mainfrom
SalmaAnany:main
Open

Salma Anany - C22- Sphinx#21
SalmaAnany wants to merge 1 commit into
Ada-C22:mainfrom
SalmaAnany:main

Conversation

@SalmaAnany

Copy link
Copy Markdown

No description provided.

@SalmaAnany SalmaAnany changed the title all waves completed with tests Salma Anany - C22- Sphinx Nov 27, 2024

@yangashley yangashley 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 job on js-adagrams!

Comment thread src/adagrams.js
@@ -1,15 +1,110 @@
const LETTER_POOL = {

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 job using capital letters to convey to other devs the value is fixed.

Comment thread src/adagrams.js
Comment on lines +38 to +39
const allLetters = []
const 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.

Nice job using const for these two variables.

Comment thread src/adagrams.js
// Implement this method for wave 1
const allLetters = []
const hand = []
for (let key of Object.keys(LETTER_POOL)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since key isn't reassigned in your loop, we should use const

Suggested change
for (let key of Object.keys(LETTER_POOL)) {
for (const key of Object.keys(LETTER_POOL)) {

Comment thread src/adagrams.js
allLetters.push(key)
}
}
for (let pepar = 0; pepar < 10; pepar++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just curious, what does pepar mean?

Comment thread src/adagrams.js
}
}
for (let pepar = 0; pepar < 10; pepar++) {
let randomIndex = Math.floor(Math.random() * allLetters.length);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
let randomIndex = Math.floor(Math.random() * allLetters.length);
const randomIndex = Math.floor(Math.random() * allLetters.length);

Comment thread src/adagrams.js
Comment on lines +91 to +92
let winningWord = ''
let winningScore = 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.

Suggested change
let winningWord = ''
let winningScore = 0
let winningWord = '';
let winningScore = 0;

While JavaScript's automatic semicolon insertion (ASI) will add a semicolon after line 4, our curriculum prefers to explicitly put them after every statement for clarity and to avoid potential issues. This can be team dependent so if you're working on JS, check in with your teammates to see how they do it.

Comment thread src/adagrams.js
Comment on lines +102 to +106
} else if (winningScore < score) {
winningScore = score
winningWord = 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.

Suggested change
} else if (winningScore < score) {
winningScore = score
winningWord = word
}
} else if (winningScore < score) {
winningScore = score;
winningWord = word;
}

Remove unnecessary blank line and add semicolons

Comment thread src/adagrams.js

}
}
return {word: winningWord, score: winningScore}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
return {word: winningWord, score: winningScore}
return {word: winningWord, score: winningScore};

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.

When breaking up an object on multiple lines, we should add indentation to make it easier to read.

Suggested change
expectScores({
"":0
});
expectScores({
"": 0
});

Or you could do it one line expectScores({"": 0});

Comment thread src/adagrams.js
for (let word of words) {
let score = scoreWord(word)
if (winningScore === score) {
if (word.length === 10 && winningWord.length !== 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.

Consider using a constant variable to reference the integer 10 to make your code more self documenting.

Maybe like TIE_BREAK_LENGTH.

Suggested change
if (word.length === 10 && winningWord.length !== 10) {
if (word.length === TIE_BREAK_LENGTH && winningWord.length !== TIE_BREAK_LENGTH) {

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