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

Module-1-2 task solutions #57

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

Conversation

balage07
Copy link

@balage07 balage07 commented Nov 23, 2020

Please review my solution for Module-1 task:

  • Changed files: module-1/classifactions.js; module-1/eucledian.js; module-1/fibonacci.js

Please review my solution for Module-2 task:

  • Changed file: module-2/test/calc.spec.js

@balage07 balage07 changed the title Module-1 task solutions Module-1-2 task solutions Nov 23, 2020
@@ -22,7 +22,30 @@ function grade(score) {
*/
// PLACE YOUR CODE BETWEEN THIS...

// ...AND THIS COMMENT LINE!

if (score < 0 || score > 100) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The solution seems to be ok but I still would suggest digesting the functions of Math, especially the Math.ceil()

Copy link
Author

Choose a reason for hiding this comment

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

Solution was reworked using Math.ceil() and Math.max() functions.


// ...AND THIS COMMENT LINE!
if (n < 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please remove the unnecessary, empty lines?

Copy link
Author

Choose a reason for hiding this comment

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

Unnecessary empty lines were removed.

//When
//Then
expect(() => c.divide(0)).to.throw("Division");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can check the exact error message as it is a static one. "Division by 0 is not possible!"

Copy link
Author

Choose a reason for hiding this comment

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

Checking of exact error message was implemented.

//When
//Then
expect(() => c.sqrt()).to.throw("Square");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please dig into the source and check if the validation of the exact message is possible?

Copy link
Author

Choose a reason for hiding this comment

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

Checking of exact error message was implemented.


});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please remove the unnecessary empty lines?

Copy link
Author

Choose a reason for hiding this comment

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

Unnecessary empty lines were removed.

…le-2/test/calc-spec.js based on Sandor Orosz's comments
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