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

Sea Turtles JS-Adagrams -Tyrah G. #103

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 68 additions & 4 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,79 @@
import { SCORE_CHART, LETTER_POOL } from "constants";

//creates letter pool
export const createLetterPool = () => {
let letterPool = [];
for (const [key, value] of Object.entries(LETTER_POOL)) {
for (let i = 0; i < value; i++) {
letterPool.push(key);
}
}
Comment on lines +5 to +10

Choose a reason for hiding this comment

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

Great way to build up the pool of tiles to draw from.

Since we modify only the contents of letterPool, it could still be declared with const since we never reassign the variable.

return letterPool;
};

//shuffles letter pool
export const drawLetters = () => {
// Implement this method for wave 1
let letterPool = createLetterPool();
let letterPoolDeepCopy = JSON.parse(JSON.stringify(letterPool));

Choose a reason for hiding this comment

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

Since you build a new `letterPool' whenever this function is called, there's no need to make another copy.

function shuffleArray(letterPoolDeepCopy) {
for (let i = 0; i < letterPoolDeepCopy; i--) {
const j = Math.floor(Math.random() * (i + 1));
[array[i], array[j]] = [array[j], array[i]];
}
}
Comment on lines +18 to +23

Choose a reason for hiding this comment

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

This function is defined, but never called. As a result, every hand we get is always A A A A A A A A A B. We now have scrabble for 🐑! 😁

I'd suggest moving this out of this function since it doesn't really need to be defined inside the drawLetters function. That would also make it easier to see that it wasn't being used yet, which is really the only issue here.

let letterBank = letterPoolDeepCopy.slice(0, 10);

Choose a reason for hiding this comment

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

This is a great way to get the first 10 items out of the list.

We can declare letterBank with const.

return letterBank;
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
for (let letters = 0; letters < input.length; letters++) {

Choose a reason for hiding this comment

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

Prefer a singular variable name. letters sounds like a collection rather than a position.

let char = input[letters];
if (lettersInHand.includes(char) === false) {

Choose a reason for hiding this comment

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

Prefer to avoid explicit comparisons to true or false. Here we would write this as if the hand does NOT include char

    if (! lettersInHand.includes(char)) {

return false;
} else if (lettersInHand.includes(char)) {

Choose a reason for hiding this comment

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

Either a letter is included or not, so this could be an else clause rather than else if

Suggested change
} else if (lettersInHand.includes(char)) {
} else {

lettersInHand.splice(char[letters], 1);

Choose a reason for hiding this comment

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

This isn't working properly. It appears to because the letters in the word and hand apparently line up in the tests.

But really, splice takes a position as its first argument. char will be a single-letter string (we looked it up from the word) and any index lookup greater than position 0 (so anything but the first letter would be passing in undefined). But whether a letter or undefined, it's having the effect of removing the first element from the list.

We can see this with this example

usesAvailableLetters('cat', ['t', 'a', 'c'])

which should be true, but returns false.

So we would need to find the position of a letter to splice out of the array before doing the splice. Check out indexOf as an alternative to includes.

Also, we don't want to modify that hand that was passed in. So if we're doing to take a destructive (to the hand) approach here, we would want to make a local copy before modifying it.

}
}
return true;
};

export const scoreWord = (word) => {
// Implement this method for wave 3
let score = 0;
//iterate through DOG
for (let char = 0; char < word.length; char++) {
let input = word[char];
if (input.toUpperCase() in SCORE_CHART) {
score += SCORE_CHART[input.toUpperCase()];
}
Comment on lines +45 to +47

Choose a reason for hiding this comment

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

Nice thought to add this bit of additional safety.

It wasn't a requirement, though in the context of these functions, we might expect to only receive words that are made of valid letter tiles.

Often, when there are a bunch of functions working together in a larger system, we'll expect the validation to have been at one central point so that we don't need to think about so many edge cases in the rest of our code.

But nice overall approach of iterating over the word, looking up the score of each letter, and adding the bonus if needed.

}
if (word.length > 6 && word.length < 11) {
score += 8;
}
return score;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 1
let maxScore = 0;
let highScoreWord = "";

for (let word = 0; word < words.length; word++) {

Choose a reason for hiding this comment

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

Nice job. Your cascading checks are pretty easy to follow!

Here, consider a iteration variable name that indicates that this is numeric (word_pos, or even simply i). word sounds like it should contain a string.

let input = words[word];
let wordScore = scoreWord(input);
if (wordScore > maxScore) {
maxScore = wordScore;
highScoreWord = input;
} else if (wordScore === maxScore) {
let wordLen = input.length;
let highScoreWordLen = highScoreWord.length;
{

Choose a reason for hiding this comment

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

No need to introduce a block here.

if (
highScoreWordLen !== 10 &&
(wordLen === 10 || wordLen < highScoreWordLen)
) {
highScoreWord = input;
}
}
}
}
return { score: maxScore, word: highScoreWord };
};
57 changes: 57 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
export const LETTER_POOL = {

Choose a reason for hiding this comment

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

Nice way to avoid cluttering up the logic file with these massive constants!

A: 9,
B: 2,
C: 2,
D: 4,
E: 12,
F: 2,
G: 3,
H: 2,
I: 9,
J: 1,
K: 1,
L: 4,
M: 2,
N: 6,
O: 8,
P: 2,
Q: 1,
R: 6,
S: 4,
T: 6,
U: 4,
V: 2,
W: 2,
X: 1,
Y: 2,
Z: 1,
};

export const SCORE_CHART = {
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,
};
8 changes: 5 additions & 3 deletions test/adagrams.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,9 @@ describe("Adagrams", () => {
});

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
expectScores({
"": 0,
});
Comment on lines +123 to +125

Choose a reason for hiding this comment

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

Nice use of the helper.

});

it("adds an extra 8 points if word is 7 or more characters long", () => {
Expand All @@ -133,7 +135,7 @@ describe("Adagrams", () => {
});
});

describe.skip("highestScoreFrom", () => {
describe("highestScoreFrom", () => {
it("returns a hash that contains the word and score of best word in an array", () => {
const words = ["X", "XX", "XXX", "XXXX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };
Expand All @@ -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);
});

describe("in case of tied score", () => {
Expand Down