Skip to content
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: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ WORKDIR /app
ARG SUBMISSION_SUBFOLDER
ADD $SUBMISSION_SUBFOLDER /app

RUN yarn add
RUN yarn install
RUN chmod +x test.sh
125 changes: 118 additions & 7 deletions src/adagrams.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,126 @@
let letterPool = new Map()

Choose a reason for hiding this comment

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

If this data structure should never be edited, we should use const to declare it. As a constant outside of a function, I would also consider using all caps naming: LETTER_POOL

Since letterPool is only accessed by the draw_letters function, it would be reasonable to place the object inside the function rather than as a constant. There are tradeoffs, the structure would clutter the function some, but it keeps the data as close as possible to where it's being used, and would mean other functions couldn't access it to accidentally alter the contents.

letterPool.set("A", 9);
letterPool.set("B", 2);
letterPool.set("C", 2);
letterPool.set("D", 4);
letterPool.set("E", 12);
letterPool.set("F", 2);
letterPool.set("G", 3);
letterPool.set("H", 2);
letterPool.set("I", 9);
letterPool.set("J", 1);
letterPool.set("K", 1);
letterPool.set("L", 4);
letterPool.set("M", 2);
letterPool.set("N", 6);
letterPool.set("O", 8);
letterPool.set("P", 2);
letterPool.set("Q", 1);
letterPool.set("R", 6);
letterPool.set("S", 4);
letterPool.set("T", 6);
letterPool.set("U", 4);
letterPool.set("V", 2);
letterPool.set("W", 2);
letterPool.set("X", 1);
letterPool.set("Y", 2);
letterPool.set("Z", 1);
Comment on lines +1 to +27

Choose a reason for hiding this comment

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

If we don't need the features of Map like maintaining the order items are added in, I suggest using a javascript object and structuring this like a dictionary for ease of reading the values.

  const letterCounts = {
    'A': 9, 
    'B': 2, 
    'C': 2, 
    ...
    'X': 1, 
    'Y': 2, 
    'Z': 1
  }



export const drawLetters = () => {
// Implement this method for wave 1
};
const letterPoolCopy = new Map(JSON.parse(JSON.stringify(Array.from(letterPool))));

Choose a reason for hiding this comment

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

These calls are pretty nested, I suggest breaking this into a couple lines for ease of reading.

With a fixed set of letters like this, we can pretty much guarantee this will always run fast, but it is worth considering that in other situations we're making 4 passes over the data structure to get a copy of the data.

Did you have issues passing the tests without the JSON parsing methods? I did a quick test and it looks like the result of Array.from(letterPool) and JSON.parse(JSON.stringify(Array.from(letterPool))) are the same.

const keys = Array.from(letterPoolCopy.keys())
let lettersDraw = [];
let count = 10;

while(count > 0){
//Find the random index
let index = Math.floor(Math.random() * keys.length);

Choose a reason for hiding this comment

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

This will give us an even chance of picking any single letter in the alphabet without going over the number of each tile we have. This is slightly different than what the README asks - we won't accurately represent the distribution of tiles because we pick a letter 1-26, when the chances of picking some letters should be higher than others. How could we update the algorithm to account for this?

let indexCounter = 0;

for (let key of letterPoolCopy.keys()) {
if (indexCounter === index) {
// If the letter already exceed it's avaliable draws, continue to do the next random draw
if(letterPoolCopy.get(key) > 0){
count--;
lettersDraw.push(key);
letterPoolCopy.set(key, letterPoolCopy.get(key)-1);
}
break;
}
indexCounter++;
}
Comment on lines +41 to +52

Choose a reason for hiding this comment

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

In a worst case scenario we'll be iterating to the back of the letterPoolCopy many times. Is there another structure we could use that would be easier to index into? What restructuring would be required?

}
return lettersDraw;
}

export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
};
// Create a copy of the lettersInHand
let lettersInHandCopy = 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 would be a great place to use the spread operator, what could that look like?

for(const letter of input){
// Check if lettersInHand includes the letter
if(lettersInHandCopy.includes(letter)){
// Find the index of the letter that needs to be removed
const removeIndex = lettersInHandCopy.indexOf(letter);
// Remove the letter from lettersInHand
lettersInHandCopy.splice(removeIndex, 1);
}else{ // If the letter in the input is not in lettersInHand, return false

Choose a reason for hiding this comment

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

Small style best practices note: we want a space on either side of the else keyword.

return false;

Choose a reason for hiding this comment

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

Nice early exit if we find a character that isn't valid.

}
}
Comment on lines +60 to +70

Choose a reason for hiding this comment

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

If we needed to reduce the time complexity of our solution, another approach could be to build a frequency map of our hand then loop over the characters in the input, checking if the character is in our frequency map, and if it is, then check the value to see if there are still tiles left in our hand for that letter.

return true;
}

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

if(word.length >= 7 && word.length <= 10){
score += 8
}

for(const ch of word){

Choose a reason for hiding this comment

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

Nice use of a for...of loop.

if(["A", "E", "I", "O", "U", "L", "N", "R", "S", "T"].includes(ch)){
score += 1
}else if(["D", "G"].includes(ch)){
score += 2
}else if(["B", "C", "M", "P"].includes(ch)){
score += 3
}else if(["F", "H", "V", "W", "Y"].includes(ch)){
score += 4
}else if(ch === "K"){
score += 5
}else if(["J", "X"].includes(ch)){
score += 8
}else{
score += 10
}
Comment on lines +83 to +97

Choose a reason for hiding this comment

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

This is completely valid, but it is a little repetitive in that we're doing something very similar in each branch of the if/else tree. I suggest creating a data structure in this function or another file that holds these values that we can reference:

const scoreChart = {
    A: 1,
    E: 1,
    I: 1,
...
    X: 8,
    Q: 10,
    Z: 10,
};

for (const letter of word) {
    if (scoreChart[letter] != undefined) {
        score += scoreChart[letter];
    }
}

}
return score
}

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let highestScoreWord = {
score: 0,
word: ""
}

for(const word of words){
const currScore = scoreWord(word);
if(currScore > highestScoreWord["score"]){
highestScoreWord["word"] = word;
highestScoreWord["score"] = currScore;
}else if(currScore === highestScoreWord["score"]){
if(highestScoreWord["word"].length === 10){
continue;
} else if(word.length === 10){
highestScoreWord["word"] = word;
continue;
}else if(word.length < highestScoreWord["word"].length){
highestScoreWord["word"] = word;
}
}
}

return highestScoreWord
};
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({});
});

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