Snow Leopards BG by bukunmig · Pull Request #47 · AdaGold/js-adagrams

bukunmig · 2022-12-09T22:20:07Z

Reviewed tests and can't see exceptions. Not sure why test is failing.

anselrognlie

Your submitted waves look good. Keep the algorithmic complexity in mind =, since just like python, many built-in JS array methods are hiding loops that we need to be aware of and may want to avoid when we're already within another loop. Please be sure to ping me when you've re-submitted with the revisions for the other waves so that I can review those as well.

anselrognlie · 2022-12-14T16:40:07Z

test/adagrams.test.js


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


anselrognlie · 2022-12-14T16:40:16Z

test/adagrams.test.js

      const correct = { word: "XXXX", score: scoreWord("XXXX") };

-      throw "Complete test by adding an assertion";
+      expect(highestScoreFrom(words)).toEqual(correct);


anselrognlie · 2022-12-14T23:09:19Z

src/adagrams.js

+  let availableLetters = [];
+  let hand = [];
+
+  for (const [key, value] of Object.entries(letterPool)) {


Consider using names more descriptive of what the key and value represent, such as letter and count.

anselrognlie · 2022-12-14T23:10:45Z

src/adagrams.js

+    let letter = availableLetters[Math.floor(Math.random() * availableLetters.length)];
+    hand.push(letter);
+    let index = availableLetters.indexOf(letter);


Alternatively, we could organize it this way and avoid needing to find the index

let index = Math.floor(Math.random() * availableLetters.length); let letter = availableLetters[index]; hand.push(letter);

anselrognlie · 2022-12-14T23:13:45Z

src/adagrams.js

+    let letter = availableLetters[Math.floor(Math.random() * availableLetters.length)];
+    hand.push(letter);
+    let index = availableLetters.indexOf(letter);
+    availableLetters.splice(index, 1);


Note that this has the same time costs as remove in python. That is, anything after the data that was removed needs to be shifted to fill the gap in the array, resulting in a O(n) time complexity with respect to the list length. Since we're working with fixed lenths (26 letters a certain number of times each, and a 10 letter hand), we dont need to worry too much about the absolute performance, but in general we want to find approaches that don't perform linear complexity operations within a loop.

anselrognlie · 2022-12-15T01:45:00Z

src/adagrams.js

 export const usesAvailableLetters = (input, lettersInHand) => {
-  // Implement this method for wave 2
-};
+  // wave 2


👀 Let me know when you've completed your rework on wave 2

anselrognlie · 2022-12-15T01:45:16Z

src/adagrams.js

 export const highestScoreFrom = (words) => {
-  // Implement this method for wave 4
-};
+  // TODO: REDO Simple option


👀 Let me know when you've completed your re-work on wave 4

anselrognlie · 2022-12-15T03:00:15Z

src/adagrams.js

+  // wave 2
+

 export const scoreWord = (word) => {


anselrognlie

Thanks for updating your submission. Looks good!

anselrognlie · 2022-12-29T01:40:48Z

src/adagrams.js

+// - Returns `true` if every letter in the `input` word is available (in the right quantities) in the `lettersInHand`
+// - Returns `false` if not; if there is a letter in `input` that is not present in the `lettersInHand` or has too much of compared to the `lettersInHand`
+
+lettersInHand = [...lettersInHand];


👍 Since we're splicing values out of the hand, we need this copy so that we don't destroy the passed in hand.

anselrognlie · 2022-12-29T01:42:54Z

src/adagrams.js

+    if(lettersInHand.includes(letter)) {
+      let indexLettersInHand = lettersInHand.indexOf(letter);
+      lettersInHand.splice(indexLettersInHand, 1);


Notice that we can use the result from indexOf itself to tell us whether the value is in the array, since it returns -1 if it is not. So we could wrrite this as

const indexLettersInHand = lettersInHand.indexOf(letter); if (indexLettersInHand !== -1) { lettersInHand.splice(indexLettersInHand, 1);

This approcah (removing the values from from the hand copy as we use them) is going to be a quadratic O(n^2) approach (since the splice, like a remove, must shift everything else forward as things get removes). We could still think about building a frequency table of letters to determine whether the word can be made with the hand in linear time.

anselrognlie · 2022-12-29T01:46:34Z

src/adagrams.js

+      winningWordAndScore ['score'] = value;
+    };
+
+    if (value === winningWordAndScore['score'] && winningWordAndScore['word'].length < 10) {


The tie breaker conditions can be an else if, since if the new score beat the old score outright, there's no need to check any of the tie breaking code.

anselrognlie · 2022-12-29T01:47:29Z

src/adagrams.js

+    score: Object.values(wordsAndScores)[0]
+  };
+
+  for (const[key, value] of Object.entries(wordsAndScores)) {


Consider using more descriptive names for the key and value (maybe word and score) so we don't have to remember that the key is the word and the value is the score.

anselrognlie · 2022-12-29T01:48:08Z

src/adagrams.js

-  // Implement this method for wave 3
+  // wave 3
+  let score = 0;
+  for (let letter of word) {


Prefer to use const for the loop control variable (letter).

bukunmig added 4 commits December 5, 2022 12:04

Update/refactor - changes to functions for waves

bf2ef3e

Update: tests

be41e35

Update: waves completed

f23eb73

Update: wave corrections

7cbf396

anselrognlie reviewed Dec 16, 2022

View reviewed changes

bukunmig added 3 commits December 17, 2022 11:41

Update: Made revision to waves 2 and wave 4.

ab42264

wave 2 and wave 4

6436685

Update: refactored code

c9f1792

anselrognlie reviewed Dec 29, 2022

View reviewed changes

Conversation

bukunmig commented Dec 9, 2022

Uh oh!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants