Skip to content

C22- Phoenix- Amber Edwards #23

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 17 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
5 changes: 4 additions & 1 deletion eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import path from "node:path";
import { fileURLToPath } from "node:url";
import js from "@eslint/js";
import { FlatCompat } from "@eslint/eslintrc";
import eslintConfigPrettier from "eslint-config-prettier";

Choose a reason for hiding this comment

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

Did you find that adding this resulted in less fighting with prettier? Personally, I prefer to use the default VS Code Js Formatter.


const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
Expand Down Expand Up @@ -44,4 +45,6 @@ export default [...compat.extends("plugin:jest/recommended", "eslint:recommended

camelcase: "error",
},
}];
},
eslintConfigPrettier,
];
14 changes: 14 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "JavaScript version of the Adagrams project",
"main": "index.js",
"scripts": {
"test": "jest",
"test": "jest --coverage",
"lint": "npx eslint",
"coverage": "open coverage/lcov-report/index.html",
"demo-game": "babel-node src/demo.js"
Expand All @@ -30,6 +30,7 @@
"babel-core": "^6.26.3",
"babel-jest": "^29.7.0",
"babel-plugin-module-resolver": "^5.0.2",
"eslint-config-prettier": "^9.1.0",
"eslint-plugin-jest": "^28.8.3",
"globals": "^15.11.0",
"jest": "^29.7.0",
Expand Down
149 changes: 144 additions & 5 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,154 @@
export const drawLetters = () => {
// Implement this method for wave 1
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 LETTER_SCORE = {
'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
};

const HAND_SIZE = 10
const MIN_BONUS_LENGTH = 7
const LENGTH_BONUS = 8

const createWeightedLetterPool = () =>{

Choose a reason for hiding this comment

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

Nice helper to expand the letter weight information into a pool that actually has the appropriate number of copies. Since this will get used in each call to drawLetters, we're free to modify the result as well if need be, since the next call will get its own fresh copy.

let weightedLetterPool = [];
for (const [letter, freq] of Object.entries(LETTER_POOL)){
for (let i=0; i<freq; i++){

Choose a reason for hiding this comment

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

Nit: Include spaces around binary operators (and throughout).

    for (let i = 0; i < freq; i++){

weightedLetterPool.push(letter);
}
}
return weightedLetterPool;
}

export const drawLetters = () => {
const hand = [];
const weightedLetterPool = createWeightedLetterPool()

while (hand.length < HAND_SIZE){

Choose a reason for hiding this comment

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

Nit: Leave a space between the conditional and the open of the block (and throughout).

  while (hand.length < HAND_SIZE) {

Choose a reason for hiding this comment

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

I'm still a bit on the fence about whether I prefer the while approach or a for loop. Since you're guaranteed to pick a letter each time through the loop, we know that we only need to run HAND_SIZE times, but from the use of the while loop, it's less clear from the condition that this will only run that many times. For that reason, I'd probably lean more towards a for loop.

let randomIdx = Math.floor(Math.random()*weightedLetterPool.length);
let randomLetter = weightedLetterPool[randomIdx];
hand.push(randomLetter);
weightedLetterPool.splice(randomIdx,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, this will need to shift values forward to close the hole left by removed item. To avoid the internal loop this requires, we can 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 hand;
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
const inputLower = input.toUpperCase();
const copy = [...lettersInHand]; // I not sure how lettersInHand is used in the program as a whole, so I made a copy

Choose a reason for hiding this comment

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

Since the approach here is destructive to lettersInHand, making a copy is essential.


for (let letter of inputLower){

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 inputLower){

if (copy.includes(letter)){
let idx = copy.indexOf(letter);
copy.splice(idx,1);
Comment on lines +91 to +93

Choose a reason for hiding this comment

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

Note that each of these calls on copy is linear, nested while iterating over the hand. This gives this logic time complexity of O(m * n), where m and n are the lengths of the input and the hand. Of course, we know those sizes are fixed for the game, (neither is longer than 10), but we should still consider the general case. If we calculated a frequency map over the hand, this would become O(m + n) instead. To know which actually has better performance for the game sizes, we would need to collect actual data, but from a complexity perspective, a frequency map approach is superior.

} else {
return false;
}
}
return true;
};





export const scoreWord = (word) => {
// Implement this method for wave 3
if (!word.trim()) {
return 0;
}
Comment on lines +106 to +108

Choose a reason for hiding this comment

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

I would leave off this trim behavior. This is granting special status to spaces at the start or end of the word (without granting the same status to spaces within the word). Personally, I'm not even a fan of this function needing to deal with case (though that's a test requirement). I would see this function as scoring a valid word, which should already have accounted for any invalid tiles or vagueries of case before getting here.

word = word.toUpperCase();
let score = 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 here

  for (const letter of word){

score += LETTER_SCORE[letter];
}
Comment on lines +111 to +113

Choose a reason for hiding this comment

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

Great use of a score lookup table to keep this simple. We could also use Array.reduce to perform this summation.

  let score = word.split('').reduce((acc, letter) => acc + LETTER_SCORE[letter], 0);

Personally, I find the loop approach you used more readable, but the reduce approach is also popular with JS programmers.

if(word.length >= MIN_BONUS_LENGTH){

Choose a reason for hiding this comment

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

Nit: Include space between if and condition.

score += LENGTH_BONUS;
Comment on lines +114 to +115

Choose a reason for hiding this comment

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

Nice use of constants to avoid magic numbers.

}
return score;
};

const getMaxScore = (words) =>{

Choose a reason for hiding this comment

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

Nice helper to find the maximum score among all the words.

let maxScore = 0;
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.

Prefer const

  for (const word of words){

maxScore = Math.max(maxScore, scoreWord(word));
};
Comment on lines +121 to +124

Choose a reason for hiding this comment

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

Finding the max is another area where JS programmers like to use reduce.

  const maxScore = words.reduce((max, word) => Math.max(max, scoreWord(word)), 0);

Alternatively, we could first translate all the words into their scores, then find the max as follows:

  const scores = words.map(scoreWord);
  const maxScore = Math.max(...scores);

return maxScore;
}

const getHighScoreWords = (words, maxScore) => {

Choose a reason for hiding this comment

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

Nice helper to filter the words down to those with the max score.

let highScoreWords = []
for (let word of words){
Comment on lines +129 to +130

Choose a reason for hiding this comment

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

Prefer const

  const highScoreWords = []
  for (const word of words){

if(scoreWord(word) === maxScore){
highScoreWords.push(word);
};
};
Comment on lines +129 to +134

Choose a reason for hiding this comment

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

This is a place where JS programmers might choose to use Array.filter

  const highScoreWords = words.filter(word => scoreWord(word) === maxScore);

return highScoreWords;
}

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let maxScore = getMaxScore(words);
let highScoreWords = getHighScoreWords(words, maxScore);
Comment on lines +139 to +140

Choose a reason for hiding this comment

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

Working with only those words having the highest score simplifies the rest of the logic in this function.


let winningWord = highScoreWords[0];

for (let word of highScoreWords){
if (winningWord.length === 10){
return {word: winningWord, score: maxScore};

Choose a reason for hiding this comment

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

As soon as we find a word that is 10 letters, we know nothing else could win, so we can exit here. Rather than duplicating the return, we could break out of the loop.

};

Choose a reason for hiding this comment

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

No semicolon here

    }

if (winningWord.length > word.length || word.length === 10){

Choose a reason for hiding this comment

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

I would probably turn this around so that it reads if the new word is shorter, rather than if the old word is longer.

    if (word.length < winningWord.length || word.length === 10){

This keeps the two conditions more consistent.

winningWord = word;
};
};
return {word: winningWord, score: maxScore};
};

10 changes: 7 additions & 3 deletions test/adagrams.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ describe("Adagrams", () => {
});

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
expect(scoreWord("")).toBe(0);
expect(scoreWord(" ")).toBe(0);
expect(scoreWord(" ")).toBe(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.

Nit: prefer ' for JS strings.

Checking for the empty string is what I would expect here. I would tend not to include the inputs that have spaces in them. I would expect scoreWord to be called on a word that already passed usesAvailableLetters. An empty string does, but a string with invalid tiles (spaces) would not. So I would stick with the first one.



});

it("adds an extra 8 points if word is 7 or more characters long", () => {
Expand All @@ -133,7 +137,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 +149,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
2 changes: 1 addition & 1 deletion test/demo/model.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Model from 'demo/model';
import Adagrams from 'demo/adagrams';

describe.skip('Game Model', () => {
describe('Game Model', () => {
const config = {
players: [
'Player A',
Expand Down