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

SCRABS #14

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

SCRABS #14

wants to merge 15 commits into from

Conversation

digivava
Copy link

Here you go! Sorry I didn't do any optional requirements this time around, but I did spend a lot of time writing the business logic from scratch. I know you said we didn't have to but I was really unhappy with my Ruby Scrabble solution and I wanted to get practice thinking through problem-solving in Javascript.

I'm glad I did! I feel like I learned a lot.
(And I tried to leave lots of useful comments this time around.) THANKS!

digivava added 15 commits June 11, 2016 12:18
…at deals with words with the same highest score
… on my own since my ruby one was very badly done. So it took time but YAY IT WORKS I THINK
…ion. Also remembered that supercalifragilisticexpialidocious is actually shorter than the longest word in the Engish language, pneumoultramicroscopicsilicovolcanoconiosis. So I changed it to that and added yaaaaaaaaay for a bit more futureproofing.
@rmtolmach
Copy link

Hey there! I'll be reviewing your code 🌻


//catches the first instance of anything that's NOT a letter
if (/[^a-z]/i.test(word) === true) {
throw "Word should only contain letters of the English alphabet.";

Choose a reason for hiding this comment

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

Can you explain lines 38 and 39 to me? How I read it is: If the word contains any letters a-z, throw the exception.

Copy link
Author

Choose a reason for hiding this comment

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

The ^ in the regex group means "not". So, anything that's not a-z. I think it's the same as doing \W in a regex...
It's just to handle those situations where people try to pass something that isn't an alphabet character.

Choose a reason for hiding this comment

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

oh, cool! My regex game is nonexistent, so thanks!

@rmtolmach
Copy link

One general comment is that many of your variable names ended up in non_javascript_style instead of javaScriptStyle 🐫 case.

@rmtolmach
Copy link

Also, your comments were helpful. Thanks!

all_scores.push(scrabble.score(word));
}
// adds them up
total_score = 0;

Choose a reason for hiding this comment

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

I think this should also not be a global variable. 🌍

@rmtolmach
Copy link

Well done, overall!

@digivava
Copy link
Author

Thank you for taking the time to look through my code! I appreciate it!

@rmtolmach
Copy link

Your welcome!
giphy

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