Skip to content
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

Kunzite - Aisha M. #141

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Kunzite - Aisha M. #141

wants to merge 3 commits into from

Conversation

aimo22
Copy link

@aimo22 aimo22 commented Jun 7, 2023

No description provided.

Copy link

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

"Z",
];

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

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.


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

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

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.

};

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

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.

return true;
} else {
return false;
}

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

score += 8;
}

for (let letterCaseSensitive of word) {

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.

} else if (letter === "Q" || letter === "Z") {
score += 10;
}
}

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

// Implement this method for wave 4
let wordsAndScores = {};

for (let word of words) {

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.

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

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.

@@ -120,7 +120,9 @@ describe("Adagrams", () => {
});

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

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.

@@ -145,7 +147,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);

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