Skip to content

Branches-Sara #44

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Branches-Sara #44

wants to merge 1 commit into from

Conversation

sarashahbaig
Copy link

JS Adagrams

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What patterns were you able to use from your Ruby knowledge to apply to JavaScript? The way to do the assignment was the same as ruby, just the syntax were different so had to be careful to do that changes.
Did you need to use different strategies to find information online about JavaScript than you would for Ruby? I used MDN instant of ruby doc.
What was a challenge you were able to overcome on this assignment? I was difficult to remember to put curly brackets, and when and were to use parentheses.
What has been interesting and positive about learning a new programming language? It was interest to know something new and learn a different language.
What is something to focus on your learnings in JavaScript in the next week? I need to be more focused and using syntax for Js, I easily mix them up and end up having error.

@jmaddox19
Copy link

JS Adagrams

What We're Looking For

Feature Feedback
General
Answered comprehension questions x
Small commits with meaningful commit messages It looks like you forgot to incrementally commit as you were working
Code Requirements
drawLetters method x
Uses appropriate data structure to store the letter distribution x
All tests for drawLetters pass One doesn't pass, because it's expecting capital letters
usesAvailableLetters method written but buggy
All tests for usesAvailableLetters pass No, none of them do
scoreWord method x
Uses appropriate data structure to store the letter scores not technically a data structure but I think it still works great :)l
All tests for scoreWord pass
Optional
highestScoreFrom method after the fixes I mention in the github pr, the code works except for when handling ties
Appropriately handles edge cases for tie-breaking logic no
All tests for highestScoreFrom pass no
Overall Good work overall :) Given usesAvailableLetters doesn't work, I think it'd be good for us to talk through that together.

} else {
return false;
}
});
},

Choose a reason for hiding this comment

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

There should be a return true; before the end of the function. As is the function is not returning anything if it hasn't already returned false.

Comment on lines +57 to +66
let word = input.toLowerCase().split("");
let sameLetters = shuffle(lettersInHand);

word.forEach(function(letter) {
if (sameLetters.includes(letter)) {
let letterLocation = sameLetters.indexOf(letter);
sameLetters.splice(letterLocation, 1);
} else {
return false;
}

Choose a reason for hiding this comment

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

I am surprised to realize 2 of the 3 tests still fail when I add the return statement mentioned below. There are several bugs in this code that it'd be good for us to talk about during our 1:1 next week.


let maxWordScore = 0;
let maxWord = "";
winningWordLetterCount = 0;

Choose a reason for hiding this comment

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

needs a let

winningWordLetterCount = 0;

words.forEach(function(word) {
if (scoreWord(word) > maxWordScore) {

Choose a reason for hiding this comment

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

This function doesn't successfully run. Each call to the scoreWord function should be called with Adagrams.scoreWord

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