Skip to content

Ada C22 Phoenix Class, Madina Dzhetegenova #42

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 4 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
134 changes: 129 additions & 5 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,139 @@
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,
};
const SCORE_CHART = {A: 1,
E: 1,
I: 1,
Comment on lines +29 to +31

Choose a reason for hiding this comment

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

If the values in the object are spread across multiple lines, list the first value indented along with the remaining values. And since JS doesn't care what order the keys are listed, it would be easier for another coder reading this if the letters were written alphabetically, as it makes it easier to confirm that all the letters are present.

const SCORE_CHART = {
  A: 1,
  B: 3,
  C: 3,

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};
export const drawLetters = () => {
// Implement this method for wave 1
const letterPool = [];
const handBank = [];

for (const [letter, count] of Object.entries(LETTER_POOL)) {
for (let i = 0; i < count; i++) {
letterPool.push(letter);
}
}
Comment on lines +59 to +63

Choose a reason for hiding this comment

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

Nice logic to build a pile of all the tiles.

for (let i = 0; i < 10; i++) {
const randomIndex = Math.floor(Math.random() * letterPool.length);
handBank.push(letterPool[randomIndex]);
letterPool.splice(randomIndex, 1);

Choose a reason for hiding this comment

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

Much like pop/remove in python, splice will need to shift values forward to close the hole left by the removed item. To avoid the internal loop this requires, we can first swap it with value to the end of the pool, then pop it. Strictly speaking, it's not even necessary to remove the value from the list, so lang as we adjusted the value we used as the list length to reflect the size of the available portion of the pool.

}

return handBank;
};


export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
for (let letter of input) {
letter = letter.toUpperCase();

if (!lettersInHand.includes(letter)) {
return false;

Choose a reason for hiding this comment

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

It looks like your editor switched to 4 space indentations occassionally. For JS, we tend to prefer smaller indentations, since more indented code tends to be common in JS. In Python, we would use deeper indentation as a hint that maybe we should do some refactoring, but that tends not to be as true in JS.

}

const index = lettersInHand.indexOf(letter);
lettersInHand.splice(index, 1);
}
Comment on lines +78 to +84

Choose a reason for hiding this comment

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

includes, indexOf, and splice are all linear with respect to the size of the hand, and are being called in an outer loop, resulting in O(m * n) time complexity, where m is the length of the word, and n is the size of the hand. While this is unlikely to be a problem for the size of our inputs, we should still consider this for our overall code approach. We would probably prefer a frequency map approach here, which would reduce the time complexity to O(m + n).

Choose a reason for hiding this comment

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

👀 Calling splice on the passed in lettersInHand modifies the passed in value. Even though the test doesn't validate that the input paramater is unchanges, we should still avoid writing our code to modify the value. This is noticeable in the demo game, since as we make successfully words from the available hand, the letters from the front are gradually removed, eventually leading to words that should be valid being reported as invalid.

Choose a reason for hiding this comment

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

Nit: This closing brace became too unindented.


return true;
};

export const scoreWord = (word) => {
// Implement this method for wave 3
};
word = word.toUpperCase();
let points = 0;

for (let letter of word) {

Choose a reason for hiding this comment

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

Prefer const

        for (const letter of word) {

points += SCORE_CHART[letter];
}

if (word.length > 6 && word.length < 11) {
points += 8;
}
Comment on lines +97 to +99

Choose a reason for hiding this comment

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

Consider creating global constants to use instead of the magic numbers, to make this code more self-documenting.

const MIN_BONUS_LENGTH = 7;
const LENGTH_BONUS = 8;

then

  if (word.length >= MIN_BONUS_LENGTH) {
    points += LENGTH_BONUS;
  }


return points;

};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let scoreMax = 0;
let wordMin = "ZZZZZZZZZZZZZZZZZ";

Choose a reason for hiding this comment

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

Rather than picking an aribtrary "min" word, if our logic gets to the point where we need to distinguich between multiple tied score words, we can pick the first word in the list of values as the word to beat. This can be handy when it's not clear what a good "min" value is. Even in this case, we have to observer that the good min word will be long (since shorter is better) but NOT have a length of exactly ten letters (since that would actually be better than any following word).

let wordsPosWin = [];

for (let word of words) {

Choose a reason for hiding this comment

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

For pretty much any for/of loop, prefer const (here and throughout)

Suggested change
for (let word of words) {
for (const word of words) {

const wordScore = scoreWord(word);
if (wordScore > scoreMax) {
scoreMax = wordScore;
}
}

for (let word of words) {
const wordScore = scoreWord(word);
if (wordScore === scoreMax) {
wordsPosWin.push(word);
}
}

if (wordsPosWin.length === 1) {
return {word: wordsPosWin[0], score: scoreMax};
}

for (let word of wordsPosWin) {
if (word.length === 10) {
return {word: word, score: scoreMax};
}

if (word.length < wordMin.length) {
wordMin = word;
}
}
return {word: wordMin, score: scoreMax};

Comment on lines +106 to +138

Choose a reason for hiding this comment

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

This whole function became intended too deeply for some reason and is using 4 space indentations.

};
9 changes: 6 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.

👍

});

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", () => {

Choose a reason for hiding this comment

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

👍

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,8 @@ describe("Adagrams", () => {
const words = ["XXX", "XXXX", "X", "XX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };

throw "Complete test by adding an assertion";
// throw "Complete test by adding an assertion";
expect(highestScoreFrom(words)).toEqual(correct);

Choose a reason for hiding this comment

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

👍

});

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