Skip to content

Kunzite - Aisha M.#141

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

Kunzite - Aisha M.#141
aimo22 wants to merge 3 commits intoAda-C18:mainfrom
aimo22:main

Conversation

@aimo22
Copy link
Copy Markdown

@aimo22 aimo22 commented Jun 7, 2023

No description provided.

Copy link
Copy Markdown

@marciaga marciaga left a comment

Choose a reason for hiding this comment

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

Great work on JS Adagrams! Overall, the translations from Python to JavaScript idioms went well. I'd suggest watching for opportunities to use const over let and invite you to explore simplifying your code when the opportunity presents itself.

Comment thread src/adagrams.js
"Z",
];

const shuffledLetterPool = letterPool.sort(() => Math.random() - 0.5);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a very interesting means of generating a random sorting of the letter pool! I think there might be a more straightforward way of accomplishing it or at least this might be the sort of code that would benefit from having a comment explaining how it works.

Comment thread src/adagrams.js

const shuffledLetterPool = letterPool.sort(() => Math.random() - 0.5);

let hand = shuffledLetterPool.slice(0, 10);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this variable isn't reassigned, you should use const over let.

Comment thread src/adagrams.js
export const usesAvailableLetters = (input, lettersInHand) => {
// Implement this method for wave 2
let loop_count = 0;
for (let letter of input) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since the variables on lines 111-112 aren't reassigned, you should use const over let.

Comment thread src/adagrams.js
return true;
} else {
return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The approach taken here definitely works! When writing code, it's always important to consider its complexity. It's a good idea to ask oneself, is there a simpler approach? For example, if you make a shallow copy of the lettersInHand argument, and perform the check to see whether a letter from the input argument is in the lettersInHand and remove it if so, otherwise if you find a letter that's not in the hand, immediately return false, you could simplify the code to something like:

const lettersInHandCopy = [...lettersInHand];
for (const letter in input.toUpperCase()) {
  if (lettersInHandCopy.includes(letter)) {
      const index = lettersInHandCopy.indexOf(letter);
      lettersInHandCopy.splice(index, 1);
  } else {
    // we've found a letter not in the hand, so we don't need to check anything else!
    return false;
  }
  // if we haven't returned yet, then all the letters are in the hand, so we can return true!
  return true;
}

Comment thread src/adagrams.js
score += 8;
}

for (let letterCaseSensitive of word) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this variable letterCaseSensitive isn't reassigned, you should use const over let.

Comment thread src/adagrams.js
} else if (letter === "Q" || letter === "Z") {
score += 10;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This if/else approach to scoring the word works perfectly well! However, it's a good idea to consider ways of simplifying code and control flow structures. For example, here, you could use a data structure that maps letters to their score values and iterate over the word to look up the score, adding it to the total, e.g.

const letterValue = {
  A: 1,
  B: 3,
  C: 3,
  D: 2,
  E: 1,
  F: 4,
  G: 2,
  H: 4,
  I: 1,
  J: 8,
  K: 5,
  L: 1,
  M: 3,
  N: 1,
  O: 1,
  P: 3,
  Q: 10,
  R: 1,
  S: 1,
  T: 1,
  U: 1,
  V: 4,
  W: 4,
  X: 8,
  Y: 4,
  Z: 10,
};

let score = 0;
for (const letter of input) {
  score += letterValue[letter];
}

Comment thread src/adagrams.js
// Implement this method for wave 4
let wordsAndScores = {};

for (let word of 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.

Since this variable word isn't reassigned, you should use const over let.

Comment thread src/adagrams.js

export const highestScoreFrom = (words) => {
// Implement this method for wave 4
let wordsAndScores = {};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This variable should be a const also. Adding key/value pairs to an object is not the same as reassigning it. This is also true for adding/removing elements from arrays.

Comment thread test/adagrams.test.js
it("returns a score of 0 if given an empty input", () => {
throw "Complete test";
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.

This passes the test, but an empty input would actually be '''. ' ' in fact represents a space.

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.

Nice job here!

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