-
Notifications
You must be signed in to change notification settings - Fork 169
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
Kunzite - Aisha M. #141
base: main
Are you sure you want to change the base?
Kunzite - Aisha M. #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on JS Adagrams! Overall, the translations from Python to JavaScript idioms went well. I'd suggest watching for opportunities to use const over let and invite you to explore simplifying your code when the opportunity presents itself.
"Z", | ||
]; | ||
|
||
const shuffledLetterPool = letterPool.sort(() => Math.random() - 0.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very interesting means of generating a random sorting of the letter pool! I think there might be a more straightforward way of accomplishing it or at least this might be the sort of code that would benefit from having a comment explaining how it works.
|
||
const shuffledLetterPool = letterPool.sort(() => Math.random() - 0.5); | ||
|
||
let hand = shuffledLetterPool.slice(0, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this variable isn't reassigned, you should use const
over let
.
}; | ||
|
||
export const usesAvailableLetters = (input, lettersInHand) => { | ||
// Implement this method for wave 2 | ||
let loop_count = 0; | ||
for (let letter of input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the variables on lines 111-112 aren't reassigned, you should use const
over let
.
return true; | ||
} else { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach taken here definitely works! When writing code, it's always important to consider its complexity. It's a good idea to ask oneself, is there a simpler approach? For example, if you make a shallow copy of the lettersInHand
argument, and perform the check to see whether a letter from the input
argument is in the lettersInHand
and remove it if so, otherwise if you find a letter that's not in the hand, immediately return false, you could simplify the code to something like:
const lettersInHandCopy = [...lettersInHand];
for (const letter in input.toUpperCase()) {
if (lettersInHandCopy.includes(letter)) {
const index = lettersInHandCopy.indexOf(letter);
lettersInHandCopy.splice(index, 1);
} else {
// we've found a letter not in the hand, so we don't need to check anything else!
return false;
}
// if we haven't returned yet, then all the letters are in the hand, so we can return true!
return true;
}
score += 8; | ||
} | ||
|
||
for (let letterCaseSensitive of word) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this variable letterCaseSensitive
isn't reassigned, you should use const
over let
.
} else if (letter === "Q" || letter === "Z") { | ||
score += 10; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if/else approach to scoring the word works perfectly well! However, it's a good idea to consider ways of simplifying code and control flow structures. For example, here, you could use a data structure that maps letters to their score values and iterate over the word to look up the score, adding it to the total, e.g.
const letterValue = {
A: 1,
B: 3,
C: 3,
D: 2,
E: 1,
F: 4,
G: 2,
H: 4,
I: 1,
J: 8,
K: 5,
L: 1,
M: 3,
N: 1,
O: 1,
P: 3,
Q: 10,
R: 1,
S: 1,
T: 1,
U: 1,
V: 4,
W: 4,
X: 8,
Y: 4,
Z: 10,
};
let score = 0;
for (const letter of input) {
score += letterValue[letter];
}
// Implement this method for wave 4 | ||
let wordsAndScores = {}; | ||
|
||
for (let word of words) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this variable word
isn't reassigned, you should use const
over let
.
export const highestScoreFrom = (words) => { | ||
// Implement this method for wave 4 | ||
let wordsAndScores = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable should be a const
also. Adding key/value pairs to an object is not the same as reassigning it. This is also true for adding/removing elements from arrays.
@@ -120,7 +120,9 @@ describe("Adagrams", () => { | |||
}); | |||
|
|||
it("returns a score of 0 if given an empty input", () => { | |||
throw "Complete test"; | |||
expectScores({ | |||
" " : 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This passes the test, but an empty input would actually be ''
'. ' '
in fact represents a space.
@@ -145,7 +147,7 @@ describe("Adagrams", () => { | |||
const words = ["XXX", "XXXX", "X", "XX"]; | |||
const correct = { word: "XXXX", score: scoreWord("XXXX") }; | |||
|
|||
throw "Complete test by adding an assertion"; | |||
expect(highestScoreFrom(words)).toEqual(correct); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job here!
No description provided.