Skip to content

JavaScript Adagram #143

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 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
118 changes: 113 additions & 5 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,123 @@
export const drawLetters = () => {
// Implement this method for wave 1
};
const letter_count = {

Choose a reason for hiding this comment

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

Consider defining letter_count outside the function to focus on the code. Your code already does the work of making a copy of data when building the expanded letter list, so relocating it won't be a problem.

"A" : 9,
"N" : 6,
"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_bank = [];
const drawn = [];

const keyList = Object.keys(letter_count);
for(let key of keyList) {

Choose a reason for hiding this comment

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

👀 Prefer const for for/of loops.

let frequency = 0;
while(frequency < letter_count[key]){
letter_bank.push(key)
frequency ++
};
Comment on lines +37 to +41

Choose a reason for hiding this comment

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

We know how many times this loop should run, so prefer using a for loop to a while loop.

    for (let i = 0; i < letter_count[key]; i += 1) {
      letter_bank.push(key)
    }; 

};

for(let i = 0; i < 10; i++) {
let select_char = Math.floor(Math.random() * (letter_bank.length -1));

Choose a reason for hiding this comment

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

random is exclusive on the upper end, so subtracting 1 from the size will never pick the final location. Also, prefer const here.

    const select_char = Math.floor(Math.random() * (letter_bank.length));

This logic selects letters with equal weighting. You have logic to reject invalid letters (letters we've already used up), but be aware that this will tend to select hands with an overrepresentation of "hard" letters, which would make it harder for the player to form words.

drawn.push(letter_bank[select_char]);
letter_bank.splice(select_char, 1);

Choose a reason for hiding this comment

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

👀splice like python's remove is linear with respect to the length of the list. This won't be a problem for the size of data we're working with, but consider using a strategy of virtually "dividing" the list into a used and unused side. Swapping a value to the used side would be constant time!

};

return drawn;
};

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
for(let char of input) {

Choose a reason for hiding this comment

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

👀 Prefer const rather than let

if(lettersInHand.includes(char)){
lettersInHand.splice(char, 1);
}else{
return false;
}
Comment on lines +55 to +59

Choose a reason for hiding this comment

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

includes and splice are both linear time, meaning that we'll be iterating over the hand as many times as there are letters in the word.

Note that splice takes the position to splice from, but that's not what the code is currently passing. It appears to work (the tests pass), but there's a fundamental problem. Take a closer look at the values being passed, and think about why that would make the tests appear to pass, then please fix the problem.

And keep in mind that rather than using splice at all, we could build frequency maps to help track the number of used characters.

};

return true;
};

export const scoreWord = (word) => {
// Implement this method for wave 3
const score_card = {

Choose a reason for hiding this comment

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

👀 Prefer to list this out alphabetically. It's hard for a human reader to be able to tell whether all the letters are there.

Choose a reason for hiding this comment

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

Consider moving the score chart out of the function to put the focus on the logic.

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

let total_score = 0 ;

for(let rawChar of word) {

Choose a reason for hiding this comment

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

Prefer const tolet

Choose a reason for hiding this comment

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

for isn't a function, so the () should stick directly to it. Leave a space after for and other control structure keywords (like if and while)

let char = rawChar.toUpperCase();

Choose a reason for hiding this comment

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

const again. Generally always try to use const and only switch back to let if JS tells us we have to (if we really do need to change what the variable refers to).

if(Object.keys(score_card).includes(char)){

Choose a reason for hiding this comment

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

👀 This will iterate over all the keys of the score chart (building a new list), then iterating through the list to look for char. Instead, we can use some variation of in, or hasOwnKey to lookup a key in an object. Or, we can try to access a key, and use falsiness (from undefined) to provide a default value such as 0.

total_score += score_card[char];
}
};
if (word.length >= 7) {
total_score += 8;
}
return total_score;
};

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let word_with_score = [];
let maxScore = 0 ;
let max_word = "";
let scoreObject = {};
for(let each_word of words){
let score = scoreWord(each_word)
if (score in scoreObject) {
scoreObject[score].push(each_word)
}
else {
scoreObject[score] = [each_word]
}
Comment on lines +96 to +102

Choose a reason for hiding this comment

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

👍 Nice way to build up lists of tied words!

if(score >= maxScore){
maxScore = score
}
}

let wordToChoose = scoreObject[maxScore][0];
if (scoreObject[maxScore].length > 1) {
if (wordToChoose.length !== 10) {
for (let each_word of scoreObject[maxScore]) {
if (each_word.length < wordToChoose.length) {

Choose a reason for hiding this comment

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

It's a little surprising to no have the 10-letter check here as well, but the logic checks out. If the first word is 10-letter, we'll never get here. If any other word is 10-letter, we'll exit the loop. Consider teasing apart some of this logic for clarity.

wordToChoose = each_word;
}
if (wordToChoose.length !== 10 && each_word.length === 10) {
wordToChoose = each_word;
break;
}
}
}
}
return { "word": wordToChoose, "score": maxScore};
};
6 changes: 3 additions & 3 deletions test/adagrams.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe("Adagrams", () => {
});

it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
expectScores({});

Choose a reason for hiding this comment

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

The expectScores helper iterates over the contents of the object we pass in. If there are no contents then no checks will be done. This should be written as

      expectScores({
        '': 0,
      });

});

it("adds an extra 8 points if word is 7 or more characters long", () => {
Expand All @@ -133,7 +133,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 +145,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