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

Jamila Octos C9 #42

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

Jamila Octos C9 #42

wants to merge 3 commits into from

Conversation

Jcornick21
Copy link

JS Scrabble

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? how to call information from where I want it and how pieces of code from various classes relate.
What was a challenge you faced in this assignment? Syntax and understanding JS syntax
Do you have any recommendations on how we could improve this project for the next cohort? don't schedule a company presentation during this project or do it at the beginning of the project and not the end.

@Jcornick21
Copy link
Author

Wrote some tests that still have bugs so I did not push those. didnt get to write all of my tests.

switch (true) {
case /[ eaotinrslu ]/.test(letter):
case /[ EAOTINRSLU]/.test(letter):
wordScore += 1

Choose a reason for hiding this comment

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

You should indent the content within a case statement

for (let letter of characters) {
switch (true) {
case /[ eaotinrslu ]/.test(letter):
case /[ EAOTINRSLU]/.test(letter):

Choose a reason for hiding this comment

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

You made the word lowercase at the start of this function so do you still need to be checking for both upper and lower cases here?


}

if (arrayOfWords.length > 0) {

Choose a reason for hiding this comment

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

Is this needed with the conditionals above?

if (this.score(word) > this.score(highScoreWrd)) {
highScoreWrd = word

} else if (this.score(word) == this.score(highScoreWrd)) {

Choose a reason for hiding this comment

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

You are potentially scoring these words multiple times. You should store the scores in a variable so you aren't duplicating this logic.

return highScoreWrd

} else if (word.length < highScoreWrd.length) {
highScoreWrd = word

Choose a reason for hiding this comment

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

Watch out! You're missing semi-colons on the majority of your js expressions here

return false
}else if (Scrabble.score(word)) {
this.plays.push(word)
let score = Scrabble.score(word)

Choose a reason for hiding this comment

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

Again, scoring the word multiple times rather than storing it in a variable

@kariabancroft
Copy link

JS Scrabble

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Not good - you should be committing much more frequently than you did in this assignment
Comprehension questions Yes
General
score calculates score, has appropriate params and return value Yes
highestScoreFrom calculates highest scoring word, has appropriate params and return value Yes
Player object
Has name and plays properties Yes
Has play, totalScore, hasWon functions Yes
Has highestScoringWord and highestWordScore functions Yes, very nice job using the existing functions you've written to solve the needs of these functions.
Overall You demonstrated your knowledge and usage of a lot of this new JS syntax. You are definitely not using semi-colons appropriately, so I'd like to see you go through this and update the code accordingly in the future.

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