Skip to content

Ev Montgomery - Cheetahs - S18#131

Open
evelynburrito wants to merge 3 commits intoAda-C18:mainfrom
evelynburrito:main
Open

Ev Montgomery - Cheetahs - S18#131
evelynburrito wants to merge 3 commits intoAda-C18:mainfrom
evelynburrito:main

Conversation

@evelynburrito
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@nancy-harris nancy-harris left a comment

Choose a reason for hiding this comment

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

Great job! See comments below for comments to make things cleaner/simpler. Also, one of the functions does not do what it is supposed to do.

For commits, make sure to commit more often and make your commits more descriptive! Instead of taking about what wave was completed, include details about what functionality was added.

Comment thread test/adagrams.test.js
Comment on lines +123 to +124
expectScores({
'': 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

Comment thread test/adagrams.test.js
const correct = { word: "XXXX", score: scoreWord("XXXX") };

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

Choose a reason for hiding this comment

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

Excellent!

Comment thread src/adagrams.js
Comment on lines +8 to +52
const letterPool = {
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 makePoolArr = function() {
const poolArr = [];
for (const [letter, quantity] of Object.entries(letterPool)) {
for (let i = 0; i < quantity; i++) {
poolArr.push(letter);
}
}

return poolArr
};

const getRandNum = function(poolArr) {
const maxLength = poolArr.length - 1;
const randNum = Math.floor(Math.random() * maxLength);
return randNum
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All of this can be done outside of the function. That way it doesn't all have to be re-created every time the drawLetters function is called. Great use of helper functions though!

Comment thread src/adagrams.js
Comment on lines +60 to +62
const letterValue = function () {
return poolArr[randNum];
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's no need for a function. Instead, you can just do hand.push(poolArr[randNum]).

Comment thread src/adagrams.js
for(let letter of input) {
let index = lettersInHand.indexOf(letter);
if(lettersInHand.includes(letter)){
lettersInHand.splice(index, 1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We do not want to modify the lettersInHand array. We should be making a copy of it at the beginning. If the user's hand loses letters every time we're just checking to see if the input has the correct letters, the user will be out of letters quickly.

Comment thread src/adagrams.js

export const scoreWord = (word) => {
// Implement this method for wave 3
const scoreChart = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again, this can be declared outside of the function so it doesn't have to be recreated every time the function is called.

Comment thread src/adagrams.js
Comment on lines +123 to +140
let score = 0
//console.log(score)
//console.log('word:', word)
if(word === "") {
return score
};

for(let letter of word.toUpperCase()) {
//console.log(letter)
score += scoreChart[letter];
//console.log(score)
}

if(word.length > 6){
score += 8;
}

return score
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent!

Comment thread src/adagrams.js
return score
};

export const highestScoreFrom = (words) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great start! This gets us to the correct answer, but requires going through the list of words multiple times. There is a way to just go through the list once. This requires having an object that keeps track of the current highest-scoring word and checking it against the next word to see if it should be replaced until you make it through the whole list.

Comment thread src/adagrams.js
Comment on lines +160 to 179
const max_score = Math.max(...scores);
//console.log(max_score)

const indices = [];
for(let i = 0; i < scores.length; i++){
if (scores[i] === max_score){
indices.push(i);
}
};
//console.log(indices)

const possibleAnswers = [];
if(indices.length === 1){
return wordsAndScores[indices[0]]
} else {
for(let index of indices){
possibleAnswers.push(wordsAndScores[index])
};
//console.log(possibleAnswers)
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Careful with indentation! It's not required to make Javascript work like in Python, but should still be done to make things easier to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants