Skip to content

Sphinx - Vlada Rapaport #44

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

Open
wants to merge 8 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
156 changes: 152 additions & 4 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,163 @@
export const drawLetters = () => {
// Implement this method for wave 1
const letterPool = {

Choose a reason for hiding this comment

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

While we use the const keyword to ensure the values cannot be reassigned, we should also use capital letters to convey to other devs the value is fixed.

Suggested change
const letterPool = {
const LETTER_POOL = {

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,
};

let letters = "";
for (const [letter, num] of Object.entries(letterPool)) {
letters += letter.repeat(num);
}

const randomlyHand = [];
const lettersCountDict = {

Choose a reason for hiding this comment

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

Suggested change
const lettersCountDict = {
const LETTERS_COUNT = {

Recall that Javascript doesn't have dictionaries. It has objects.

A: 0,
B: 0,
C: 0,
D: 0,
E: 0,
F: 0,
G: 0,
H: 0,
I: 0,
J: 0,
K: 0,
L: 0,
M: 0,
N: 0,
O: 0,
P: 0,
Q: 0,
R: 0,
S: 0,
T: 0,
U: 0,
V: 0,
W: 0,
X: 0,
Y: 0,
Z: 0,
};

while (randomlyHand.length < 10) {

Choose a reason for hiding this comment

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

Prefer to reference the integer 10 with a variable. This makes your code more readable/self documenting because it clues other devs in to the meaning of 10.

Suggested change
while (randomlyHand.length < 10) {
const HAND_LENGTH = 10;
while (randomlyHand.length < HAND_LENGTH) {

const i = Math.floor(Math.random() * letters.length);

Choose a reason for hiding this comment

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

Prefer more descriptive name for this variable, maybe like randomIndex

const selectedLetter = letters[i];

if (lettersCountDict[selectedLetter] < letterPool[selectedLetter]) {
lettersCountDict[selectedLetter]++;
randomlyHand.push(selectedLetter);
}
}

return randomlyHand;
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
let lettersInHandCopy = [];

Choose a reason for hiding this comment

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

Suggested change
let lettersInHandCopy = [];
const lettersInHandCopy = [];

A list that is declared with const can still be updated. We won't be able to change what lettersInHandCopy references though, which is what we want. We should always prefer to use const whenever possible

let wordLower = input.toLowerCase();

for (let letter of lettersInHand) {
lettersInHandCopy.push(letter.toLowerCase());
}

for (let letter of wordLower) {
Comment on lines +81 to +87

Choose a reason for hiding this comment

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

2 notes:

  1. If your looping variable letter is not updated in the loop, then we should use const instead.

  2. To make your code more concise, call toLowerCase() when you need it:

Suggested change
let wordLower = input.toLowerCase();
for (let letter of lettersInHand) {
lettersInHandCopy.push(letter.toLowerCase());
}
for (let letter of wordLower) {
for (const letter of lettersInHand.toLowerCase()) {
lettersInHandCopy.push(letter);
}
for (const letter of input.toLowerCase()) {

if (!lettersInHandCopy.includes(letter)) {
return false;
} else {
lettersInHandCopy.splice(lettersInHandCopy.indexOf(letter), 1);

Choose a reason for hiding this comment

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

splice like python's remove has linear time complexity with respect to the length of the list. This won't be a problem for the size of data we're working with, but consider using a strategy of virtually "dividing" the list into a used and unused side. Swapping a value to the used side would be constant time!

}
}
return true;
};

export const scoreWord = (word) => {
// Implement this method for wave 3
const scoreDict = {

Choose a reason for hiding this comment

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

Suggested change
const scoreDict = {
const SCORE_DICT = {

A: 1,
E: 1,
I: 1,
O: 1,
U: 1,
L: 1,
N: 1,
R: 1,
S: 1,
T: 1,
D: 2,
G: 2,
B: 3,
C: 3,
M: 3,
P: 3,
F: 4,
H: 4,
V: 4,
W: 4,
Y: 4,
K: 5,
J: 8,
X: 8,
Q: 10,
Z: 10,
};
const bonusLengthMin = 7;
const bonusLengthMax = 10;
const bonusPoints = 8;
Comment on lines +126 to +128

Choose a reason for hiding this comment

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

Use all caps for naming thees constant variables


let totalScore = 0;

for (const letter of word.toUpperCase()) {
totalScore += scoreDict[letter];
}

if (word.length >= bonusLengthMin && word.length <= bonusLengthMax) {
totalScore += bonusPoints;
}

return totalScore;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
const tieBreakingLength = 10;

Choose a reason for hiding this comment

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

Suggested change
const tieBreakingLength = 10;
const TIE_BREAKING_LENGTH = 10;

let maxWord = words[0];
let maxScore = scoreWord(maxWord);

for (let i = 1; i < words.length; i++) {

Choose a reason for hiding this comment

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

Prefer for/of loop instead because we don't need i to access the letter we want to score.

const word = words[i];
const score = scoreWord(word);

if (score > maxScore) {
maxScore = score;
maxWord = word;
} else if (score === maxScore && maxWord.length !== tieBreakingLength) {
if (word.length === tieBreakingLength || word.length < maxWord.length) {
maxWord = word;
}
}
}

return { word: maxWord, score: maxScore };
};
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,
});
});

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
Loading