-
Notifications
You must be signed in to change notification settings - Fork 103
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
Madi Jackson #87
base: main
Are you sure you want to change the base?
Madi Jackson #87
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.
Nice work on your first solo javascript project 🎉 I've added some questions & comments below, please take a look when you have a moment and reach out here or on Slack if I can clarify anything =]
Z: 1, | ||
}; | ||
const LETTER_SCORE = { |
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.
Tiny best practices note: The spacing between data structures/functions and indentation isn't consistent across the file, just something to keep in mind to take a look for before opening PRs.
for (let [key, value] of Object.entries(LETTER_POOL)){ | ||
fullHand.push(...Array(value).fill(key)); | ||
} |
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 combination of iteration and the spread operator to create the letter pool to draw from.
let finalWord = testWord.join(''); | ||
if (finalWord === 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.
There's a bug caused by these lines that I'd like you to try troubleshooting.
What happens if lettersInHand
has more than the exact same count of a letter in input
, or the letters occur in a different order in lettersInHand
than the order in input
? I've written a couple tests you can add to the suite to try this out:
it("returns true when word contains letters in the drawn letters out of order", () => {
const drawn = ["G", "D", "O", "X", "X", "X", "X", "X", "X", "X"];
const word = "DOG";
const isValid = usesAvailableLetters(word, drawn);
expect(isValid).toBe(true);
});
it("returns true when drawn letters contains duplicates of letters in word", () => {
const drawn = ["D", "O", "G", "G", "X", "X", "X", "X", "X", "X"];
const word = "DOG";
const isValid = usesAvailableLetters(word, drawn);
expect(isValid).toBe(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.
We could remove the else block here and unindent return false;
without changing the function's outcome.
} | ||
} | ||
|
||
for (let [key, value] of Object.entries(lettersInHand)){ |
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.
While javascript allows this syntax, looking at the tests, lettersInHand
is an array of strings. To avoid confusion about the kind of data type we're iterating over and remove variables like key
that aren't being used, a for-of loop would be a better fit:
for (let letter of lettersInHand) {
...
let winnersList= {}; | ||
let lengthList = {}; |
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.
We could remove these dictionaries since it looks like they're only referenced in the commented out code.
let winnersList= {}; | ||
let lengthList = {}; | ||
|
||
for (let letter 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.
In this case we're iterating over a list of words rather than letters, I suggest updating the variable name letter
to better reflect what it represents.
let wordDict = {}; | ||
input.toUpperCase(); | ||
|
||
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.
Love the thought to use a frequency map here!
// } | ||
} | ||
let counter = 0 | ||
let contestants = '' |
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 contestants
will contain a single word, it's a naming convention to use a singular name like contestant
to represent that.
if (value > counter){ | ||
counter = value; | ||
contestants = key; | ||
}else if (value === counter){ |
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.
Small style best practice: we should include a space after a closing brace before any following key words.
let testWord = []; | ||
let wordDict = {}; |
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.
We should prefer using const anywhere we don't need to reassign values - Lists & dictionaries are mutable, we can declare them as const and still add new values, we just then can't re-assign the variable to a new array or dictionary.
Finished adagrams js!