Skip to content

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

// ...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.

if (n >= 0) {

// ...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.

const c = calc(5);
//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.

const c = calc(-3);
//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