Skip to content

Pheonix class: Madeline Bennett #31

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
12 changes: 6 additions & 6 deletions package-lock.json

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

161 changes: 156 additions & 5 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,166 @@
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.

Consider moving the dictionary of letters to be a global constant. This is pretty common to do with large data values, so that the body of the function isn't dominated with just the data, which we have to scroll past to find the actual logic. Also, while it would only be a small cost, JS does recreate the dictionary over and over each time drawLetters is called, whereas if we move it outside the function, it will be initialized only once.

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 drawnLetters = [];
for (let iteration = 0; iteration < 10; iteration++) {

Choose a reason for hiding this comment

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

For a "throw away" iteration variable, just stick with i. Giving it any other name will just make the person reading the code wonder why a different name was used.

const keys = Object.keys(letterPool);
const key = keys[getRandomInt(0, keys.length)];
letterPool[key]--;
drawnLetters.push(key)
if (letterPool[key] == 0) {
delete letterPool[key];
}
}
Comment on lines +32 to +39

Choose a reason for hiding this comment

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

I would consider this a regression from your python approach. Originally, you built a tile pool containing as many copies of each letter as was required by the count data. This accurately modelled the process of picking tiles from a pile. This revised approach does not accurately represent that process. Since a letter is picked first, this means that the "hard" letters, which have fewer tiles, will be over represented in the picked hands. They still won't appear more times than is possible, but compared to hands that pick from a pile, there will be more "hard" letters. Futher, the likelihood to pick any particular letter changes as each tile is picked in the pile approach, but this will only adjust the likelihood when an entire letter has been cleared out (and it still doesn't match the pile approach).

The pile approach to picking tiles (as you used in your python version) provides the more accurate parallel to a physical game, so I would stick with that approach.

return drawnLetters;

};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
};

export const usesAvailableLetters = (input, lettersInHand) => {
if (input.length > 10) {
return false;
}
Comment on lines +46 to +48

Choose a reason for hiding this comment

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

This wasn't a specified check, and is outside the scope of "using the available letters". I would expect a max length check to occur elsewhere in the game logic. This function should only focus on where the suppliedd word (whatever its length) can be made from the supplied collection of tiles.

const lettersInHandOccurenceDictionary = arrayToOccurenceDict(lettersInHand)

Choose a reason for hiding this comment

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

Nice use of a helper function to calculate a frequency map of the tiles.

for (let index = 0; index < input.length; index++) {
let key = input[index];
if (lettersInHandOccurenceDictionary[key] === undefined) {
return false
}
else if (lettersInHandOccurenceDictionary[key] === 0) {
return false
}
Comment on lines +52 to +57

Choose a reason for hiding this comment

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

Notice that both undefined and 0 are falsy, so we could simplify this as

    if (!lettersInHandOccurenceDictionary[key]) {
      return false;
    }

Keep an eye out for semicolons as well.

else {
lettersInHandOccurenceDictionary[key]--;
}
}
return true;
};

Choose a reason for hiding this comment

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

Even though JS doesn't rely on indentation for its flow control, we should still be careful to indent properly to help with the code readability. This line should be fully unindented.





export const scoreWord = (word) => {
if (word.length === 0) {
return 0;
}
Comment on lines +69 to +71

Choose a reason for hiding this comment

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

This special case isn't necessary. An empty word will start with a score of 0, and not have anything added to it, resulting in a final score of 0. Avoid adding special cases to code that already handles it properly in the main logic.

let wordScore = 0;
const letterOccurenceDict = arrayToOccurenceDict(word.toUpperCase())

Choose a reason for hiding this comment

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

Converting the word into a frequency map in this function doesn't improve our time complexity in this method. We need to iterate over the word to build the map, but we can also score the word in a single iteration. So I'd avoid introducing the complication of using a frequency map.

const keys = Object.keys(letterOccurenceDict);
for (let index = 0; index < keys.length; index++) {
let key = keys[index];
wordScore = wordScore + (getLetterValue(key,letterOccurenceDict))

Choose a reason for hiding this comment

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

The main thing that getLetterValue is responsible for is getting the score for a letter. This could be handled (and is in the helper) by a object map lookup.

The other thing the function is doing is multiplying the looked up score by the count obtained from the frequency map. This moves some of the knowledge about how to tally this score from this function to the helper. That is, there are now two functions with intimate knowledge of how to perform this scoring.

I would prefer to keep the multiplication responsibility here, so that the extra data doesn't need to be passed in to the helper. Then, the only thing being done in the helper is the object map lookup. We could still keep that in the helper to insulate the main function from needing to know how to get the score for a tile, or we could bring that lookup back to this function and remove the helper.

If instead of working from a frequency map, we just iterated over the word, then no multiplication would be needed, and we could sum up the individual tile scores directly. This is how you approached it in your python code, and I would say the python approach was easier to understand and maintain than this JS version would be.

}
if (word.length >= 7) {
wordScore += 8
}
Comment on lines +79 to +81

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) {
    wordScore += LENGTH_BONUS;
  }

console.log(`word score is `, wordScore)

Choose a reason for hiding this comment

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

Nit: remove debug output.

return wordScore




// Implement this method for wave 3
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let topScoreNum = 0;
let topScoreWord = '';

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

Choose a reason for hiding this comment

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

Nit: Be midnful of spacing aorund binary operators.

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

let currentWord = words[index];
let currentScore = scoreWord(currentWord);
if (currentScore > topScoreNum){
// console.log(`current score was greater than top score`)
topScoreNum = currentScore;
topScoreWord = currentWord;
}
else if (currentScore === topScoreNum) {
// console.log(`current score equal to top score`)
if (currentWord.length === 10 && topScoreWord.length !== 10) {
// console.log(`current word is 10 top is not, current wins`)
topScoreNum = currentScore;
topScoreWord =currentWord;

Choose a reason for hiding this comment

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

Nit: spacing around assignment

        topScoreWord = currentWord; 

}
else if (currentWord.length < topScoreWord.length && topScoreWord.length !== 10) {
topScoreNum = currentScore;
topScoreWord = currentWord;
}
Comment on lines +103 to +113

Choose a reason for hiding this comment

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

Notice that both of the nested conditions requires that the winning word not have a length of 10. We could move those checks up to the outer condition, and simplify the nested checks.

    else if (currentScore === topScoreNum && topScoreWord.length !== 10) {
      if (currentWord.length === 10) {
        topScoreNum = currentScore; 
        topScoreWord = currentWord; 
      } else if (currentWord.length < topScoreWord.length) {
        topScoreNum = currentScore; 
        topScoreWord = currentWord; 
      }
    }

We can also observe that the body of each of those checks is identical, so they could be combined into a sungle check. We could even squish them all together in that outer else, but that might be getting hard to read.

}

}
console.log({word: topScoreWord, score: topScoreNum})

Choose a reason for hiding this comment

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

Nit: remove debug output.

return {word: topScoreWord, score:topScoreNum}
};


//// helper functions///////

const arrayToOccurenceDict = (array) => {

Choose a reason for hiding this comment

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

Nice helper to build a frequency map from something iterable.

const occurenceDictionary = {}
for (let index = 0; index < array.length; index ++){
let key = array[index];
if (occurenceDictionary[key] !== undefined) {
occurenceDictionary[key]++
} else {
occurenceDictionary[key] = 1
}
}
return occurenceDictionary
}

const getRandomInt = (min, max) => {

Choose a reason for hiding this comment

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

👀 Because the previous closing brace was improperly indented, that's thrown off the formatting of everything that follows.

const minCeiled = Math.ceil(min);
const maxFloored = Math.floor(max);
Comment on lines +138 to +139

Choose a reason for hiding this comment

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

It's surprising to me that a helper function that is supposed to pick an int doesn't just require that the caller already ensure that inputs are both ints. If it is going to force the inputs to be ints, I'd expect them to both have the same transformation applied (typically, this would be a truncation (Math.trunc).

return Math.floor(Math.random() * (maxFloored - minCeiled) + minCeiled); // The maximu
}

const getLetterValue = (key, letterOccurenceDict) => {
const lettersAndValues = {
//1 point
'A' : 1, 'E' : 1, 'I' : 1, 'O' : 1 , 'U' : 1, 'L' : 1, 'N' : 1, 'R' : 1, 'S' : 1, 'T' : 1,
//2 points
'D' : 2, 'G' : 2,
//3 points
'B' : 3, 'C' : 3, 'M' : 3, 'P' : 3,
//4 points:
'F' : 4, 'H' : 4, 'V' : 4, 'W' : 4, 'Y': 4,
//5 points:
'K' : 5,
//6 points:
'J' : 8, 'X' : 8,
//7 points:
Comment on lines +155 to +157

Choose a reason for hiding this comment

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

Lying comments 😭

'Q' : 10, 'Z' : 10
Comment on lines +145 to +158

Choose a reason for hiding this comment

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

Since JS won't care what order the letters are listed, prefer listing them in a way that's easier for humans to check that everything is present and accounted for. Writing the letters alphabetically would make it easier for the reader to confirm that all letters are listed with no gaps.

}
if (isNaN(lettersAndValues[key])){
return letterOccurenceDict[key]
}
Comment on lines +160 to +162

Choose a reason for hiding this comment

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

This is puzzling to me. My interpretation of the intent is that if the specified letter doesn't appear in the score data, that it will use the number of occurrences as the score. Why would an "unscored" letter be worth any score?

We didn't have any test cases where you would receive an unscored letter, but if I were in that situation, I would either ignore the invalid letter (give it a 0 score), or raise some sort of error.

let letterValue = lettersAndValues[key];
let letterOccurence = letterOccurenceDict[key];
return letterValue * letterOccurence
};
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.

👍

});

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

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
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