Ocelots --- Monica Bao #16

Yf-Monica-Bao · 2023-01-23T00:58:29Z

No description provided.

kelsey-steven-ada

Looks good! I’ve added some suggestions, let me know if there's anything I can clarify ^_^

kelsey-steven-ada · 2023-01-26T03:53:28Z

src/adagrams.js

+let letterPool = new Map() 
+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);


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 }

kelsey-steven-ada · 2023-01-26T03:56:29Z

src/adagrams.js

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


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.

kelsey-steven-ada · 2023-01-26T03:58:30Z

src/adagrams.js

@@ -1,15 +1,126 @@
+let letterPool = new Map() 


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.

kelsey-steven-ada · 2023-01-26T04:19:46Z

src/adagrams.js

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


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?

kelsey-steven-ada · 2023-01-26T04:24:01Z

src/adagrams.js

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


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?

kelsey-steven-ada · 2023-01-26T05:41:24Z

src/adagrams.js

+      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 


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

kelsey-steven-ada · 2023-01-26T05:41:59Z

src/adagrams.js

+      // Remove the letter from lettersInHand 
+      lettersInHandCopy.splice(removeIndex, 1);
+    }else{ // If the letter in the input is not in lettersInHand, return false 
+      return false;


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

kelsey-steven-ada · 2023-01-26T05:45:11Z

src/adagrams.js

+  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 
+      return false;
+    }
+  } 


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.

kelsey-steven-ada · 2023-01-27T04:30:19Z

src/adagrams.js

+    score += 8 
+  }
+
+  for(const ch of word){


Nice use of a for...of loop.

kelsey-steven-ada · 2023-01-27T04:34:38Z

src/adagrams.js

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


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

Monica-Yfb added 5 commits January 19, 2023 14:54

Wave 1 completed

aec06ec

Wave4 completed

16c2881

Wave2 completed

b08d92a

Wave3 completed

c792f19

Project completed

6895f06

kelsey-steven-ada reviewed Jan 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ocelots --- Monica Bao #16

Ocelots --- Monica Bao #16

Uh oh!

Yf-Monica-Bao commented Jan 23, 2023

Uh oh!

kelsey-steven-ada left a comment

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 26, 2023

Uh oh!

kelsey-steven-ada Jan 27, 2023

Uh oh!

kelsey-steven-ada Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ocelots --- Monica Bao #16

Are you sure you want to change the base?

Ocelots --- Monica Bao #16

Uh oh!

Conversation

Yf-Monica-Bao commented Jan 23, 2023

Uh oh!

kelsey-steven-ada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants