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

Beylul Kbreab #25

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

Beylul Kbreab #25

wants to merge 5 commits into from

Conversation

Bella8
Copy link

@Bella8 Bella8 commented Nov 18, 2016

Finished with the basic requirements of the project and tested it. I have commented out the tests.

this.word = word.split("");
var score = 0;
if (this.word.length == 7) {
score = 50;

Choose a reason for hiding this comment

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

I like the way you reassigned the base score based on the length

}
}

for (var i = 0; i < arrayOfWords.length; i++) {

Choose a reason for hiding this comment

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

This is a bit redundant since it is repeating almost all of the same logic in the loop above. Any ideas on how to consolidate?

}

if(scoredWords.length > 1)
{

Choose a reason for hiding this comment

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

JS syntax is a bit weird, but we do expect the { to be on the same line as the if and the else

var totalScore = 0;
for(var i = 0; i < this.plays.length; i++){
totalScore += this.scrabble.score(this.plays[i]);
console.log(this.scrabble.score(this.plays[i]));

Choose a reason for hiding this comment

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

This is good for debugging but it results in duplicating the score logic multiple times. I'd recommend assigning this value to a variable and then printing it our for performance purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants