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

finally done with adagrams js #86

Open
wants to merge 1 commit 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
137 changes: 131 additions & 6 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,140 @@
const LETTER_POOL = {

Choose a reason for hiding this comment

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

Great use of upper snake case for your constants!

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


function getRandomProperty() {

Choose a reason for hiding this comment

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

I love this helper function!

const keys = Object.keys(LETTER_POOL);

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

let drawnLetters = [];

Choose a reason for hiding this comment

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

Because drawnLetters is only used in the drawLetters function, I would make the variable local to the drawLetters function. Also, we add values to it, but we never reassign the reference to drawnLetters, so const may be the better choice here! Always prefer const over let. If you accidentally try to reassign a variable that was declared with const, then the JS interpreter will let you know and you can change it to let.


export const drawLetters = () => {
// Implement this method for wave 1
while (drawnLetters.length < 10) {
let myCurrentKey = getRandomProperty();
let myCurrentValue = LETTER_POOL[myCurrentKey];
if (myCurrentValue !== 0) {
drawnLetters.push(myCurrentKey);
LETTER_POOL[myCurrentKey] = myCurrentValue - 1;
}
if (LETTER_POOL[myCurrentKey] === 0) {
delete LETTER_POOL[myCurrentKey];
}
}

return drawnLetters;

Choose a reason for hiding this comment

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

This function looks good but I have two suggestions:

  1. I would make a copy of LETTER_POOL because as it stands, you are destroying LETTER_POOL as you remove letters.
  2. You don't ever account for the different numbers of letters, so the randomness accounts for the 26 different keys, but it weights them each equally. To get around this, we would want to create some sort of structure that holds all the different letters and randomly picks one from there!

};



export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
};
let bankCopy = JSON.parse(JSON.stringify(lettersInHand))

Choose a reason for hiding this comment

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

This is a good way to copy a list. One thing to think about however is that the list we are copying simply contains letters, not objects, so a shallow copy using the spread operator will work fine here!

 bankCopy = [...lettersInHand];

for (let letter of input) {
if (!bankCopy.includes(letter.toUpperCase())){
return false
} else if (bankCopy.includes(letter.toUpperCase())) {
bankCopy = bankCopy.filter(value => value !== letter)

Choose a reason for hiding this comment

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

The Javascript filter function runs in linear time. I wonder if there is a way to utilize objects to make this removal happen in constant time?

}
}
return true
};


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

Choose a reason for hiding this comment

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

lengthOfWord never changes, so you could absolutely make this a const!

const letterValueDict = {

Choose a reason for hiding this comment

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

Great creation of a local const!

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, V: 4,
W: 4, Y: 4, K: 5, J: 8, X: 8, Q: 10,
Z: 10, H:4
};

if (word.length === 0){
return 0;
}

if(lengthOfWord >= 7){

Choose a reason for hiding this comment

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

Best practice is to place a space after the if keyword to avoid looking like a function call!

score += 8;
}

for (let i = 0; i < lengthOfWord; i++) {
const alpha = word[i].toUpperCase();
score += letterValueDict[alpha];
}

return score;

}


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

for (const word of words) {
wordDict[word] = scoreWord(word);
}

maxValue = Math.max(...Object.values(wordDict));


for (const [key, value] of Object.entries(wordDict)) {
if(value === maxValue) {
listOfMaxKeys.push(key);
}
}

if (listOfMaxKeys.length > 1) {
let tenLetterWordsList = [];
let nonTenLetterWordsList = [];

for (const word of listOfMaxKeys) {
if (word.length === 10) {
tenLetterWordsList.push(word);
} else {
nonTenLetterWordsList.push(word);
}
}

if (tenLetterWordsList.length > 0) {
return { word: tenLetterWordsList[0], score: wordDict[tenLetterWordsList[0]] };
} else {
let sortedNonTenLetterWordList = nonTenLetterWordsList.sort((a, b) => a.length - b.length);
let shortestWord = sortedNonTenLetterWordList[0];
return { word: shortestWord, score: wordDict[shortestWord] };
}
}

return { word: listOfMaxKeys[0], score: wordDict[listOfMaxKeys[0]] };
};

Choose a reason for hiding this comment

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

I understand the concept behind what you are trying to do here! Using objects to separate out each step makes sense, but it starts to get tedious and more complex when we do that. Since we are returning an object with word and score as keys, why not create the one object that has the first word and the first word score as values? From there, we can run through the rest of the wordList and make the comparisons we need in place! This will drastically decrease the complexity and code you need to use! An example is below:

 export const highestScoreFrom = (words) => {
   let winningWord = {
     word: null,
     score: null
   }

   for (let word of words){

     let thisWord = {
       word: word,
       score: scoreWord(word)
     }

     if (thisWord.score > winningWord.score){
       winningWord = thisWord
     } else if (thisWord.score === winningWord.score){
       const isBestTen = winningWord.word.length === 10;
       const isWordTen = thisWord.word.length === 10;
       const isWordShorter = thisWord.word.length < winningWord.word.length;

       if ((isWordTen && !isBestTen) || (isWordShorter && !isBestTen)){
         winningWord = thisWord;
       }
     }
   }
   return winningWord;
 }


13 changes: 9 additions & 4 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,19 +135,22 @@ 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") };

console.log('!!!!!!!!!!!!!!!')
console.log(highestScoreFrom(words))
console.log(correct)
expect(highestScoreFrom(words)).toEqual(correct);
});

it("accurately finds best scoring word even if not sorted", () => {
const words = ["XXX", "XXXX", "X", "XX"];
const correct = { word: "XXXX", score: scoreWord("XXXX") };

throw "Complete test by adding an assertion";
const result = highestScoreFrom(words);
expect(result).toEqual(correct);
});

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