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

zoisite gwen #57

Open
wants to merge 2 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
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
"babel-core": "^7.0.0-bridge.0",
"babel-jest": "^24.8.0",
"babel-plugin-module-resolver": "^3.2.0",
"eslint": "^8.41.0",
"eslint-plugin-jest": "^27.2.1",
"jest": "^24.8.0",
"regenerator-runtime": "^0.12.1"
},
Expand Down
184 changes: 181 additions & 3 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,193 @@



Comment on lines +1 to +3

Choose a reason for hiding this comment

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

Delete unnecessary blank lines

const letterPool = {

Choose a reason for hiding this comment

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

We should name the variable with all caps and underscores to indicate that this is a constant variable 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 pointValues = {

Choose a reason for hiding this comment

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

Constant variable should be named with all caps like POINT_VALUES

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

export const drawLetters = () => {
// Implement this method for wave 1
let tiles = [];
let letterPoolList = [];
let letterFrequency = {};
Comment on lines +63 to +65

Choose a reason for hiding this comment

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

These three variables are not re-assigned and should be declared with const, instead of let

let count = 0;

for (let letter of Object.keys(letterPool)){

Choose a reason for hiding this comment

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

Use const instead of let when declaring letter in the for loop since the variable is never re-assigned.

for (let i= 0; i <= letterPool[letter] - 1; i++){

Choose a reason for hiding this comment

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

Nice job representing the distribution of letters by pushing each letter in X times.

Nitpick: there should be a whitespace after i let i = 0 for consistency. Be sure to review your own code before opening a PR to check that spacing/indentation/use of blank lines follow recommendations per a language's style guide.

letterPoolList.push(letter);
}
}

while (count <= 9){
const random = () => {

Choose a reason for hiding this comment

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

A more descriptive name for this method would be getRandomLetter. Also prefer this helper method to be written outside of drawLetters. As is, every time the while loop executes you're having to re-define the method.

return letterPoolList[Math.floor(Math.random()* letterPoolList.length)];
};

let randomLetter = random();

Choose a reason for hiding this comment

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

👍 we do need let here since you might reassign the variable on line 85




if (randomLetter in Object.keys(letterFrequency)){
if ((letterFrequency[randomLetter] + 1) > letterPool[[randomLetter]]){
randomLetter = random();
} else {
letterFrequency[randomLetter] ++;
}
} else {
letterFrequency[randomLetter] = 1;
}
tiles.push(randomLetter);
count++;
}

return tiles;



};



export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
let lettersInHandFreq = {};
let inputFreq = {};
Comment on lines +105 to +106

Choose a reason for hiding this comment

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

Use const

input.toUpperCase();

for(let letter of lettersInHand){
if(letter in lettersInHandFreq){
Comment on lines +109 to +110

Choose a reason for hiding this comment

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

Nitpick: there should be a whitespace between for/if and the opening parenthesis.

Use const when defining letter

lettersInHandFreq[letter] += 1
} else {
lettersInHandFreq[letter] = 1
}
}

for(let char of input){
if(char in lettersInHandFreq){
Comment on lines +117 to +118

Choose a reason for hiding this comment

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

Watch for whitespace and use const for defining char

if(char in inputFreq){
inputFreq[char] += 1
} else {
inputFreq[char] = 1
}
} else {
return false;
}
}

for(let letter of Object.keys(inputFreq)){
if(inputFreq[letter] > lettersInHandFreq[letter]){
return false;
}
}
return true;
Comment on lines +117 to +134

Choose a reason for hiding this comment

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

How could you simplify the logic a little bit in usesAvailableLetters without creating inputFreq ?


};

export const scoreWord = (word) => {
// Implement this method for wave 3
let totalPoints = 0;


if (word.length >= 7) {
totalPoints += 8;
}

for(let index in word){

Choose a reason for hiding this comment

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

Since the variable index is not being reassigned in the for loop, we should use const here. Also, it's more descriptive to name the variable letter since the elements we're iterating over in word are the individual letters not a numerical index value

let letterPointValue = pointValues[(word[index]).toUpperCase()]

Choose a reason for hiding this comment

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

Use const instead of let for letterPointValue since it's not being reassigned. Generally, we should use const and only change to let when we see that we cannot reassign a value when we need to.

totalPoints += letterPointValue;
}

return totalPoints;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let wordPoints = {};
let pointList = [];

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.

Nitpick - proper whitespaces missing

let points = scoreWord(word)
Comment on lines +155 to +159

Choose a reason for hiding this comment

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

Use const

pointList.push(points)
if(points in wordPoints){

Choose a reason for hiding this comment

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

Nitpick - proper white spaces missing for this if statement

wordPoints[points].push(word)
} else {
wordPoints[points] = [word]
}
}

let highestPoints = Math.max(...pointList);
let highestScoredWords = wordPoints[highestPoints];

let currentWinner = highestScoredWords[0]

if(highestScoredWords.length === 1){
return {'word' : currentWinner, 'score': highestPoints};
}

let range = highestScoredWords.length - 1;

Choose a reason for hiding this comment

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

Use const


for(let i=1; i <= range; i++){
if(currentWinner.length === 10){
return {'word' : currentWinner, 'score': highestPoints};
} else if((highestScoredWords[i]).length === 10){
return {'word' : highestScoredWords[i], 'score': highestPoints};
} else if(currentWinner.length > (highestScoredWords[i]).length){
Comment on lines +179 to +184

Choose a reason for hiding this comment

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

Fix up whitespaces around these statements.

currentWinner = highestScoredWords[i]
}

}
return {'word' : currentWinner, 'score': highestPoints}


};

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

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
const word = '';
expect(scoreWord(word)).toBe(0);

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 +134,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 +146,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
Loading