-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Solution #2931
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
base: master
Are you sure you want to change the base?
Solution #2931
Conversation
DemonWayne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay, specified some minor problems below
| add(number) { | ||
| return this.result += number; | ||
| }, | ||
|
|
||
| subtract(number) { | ||
| return this.result -= number; | ||
| }, | ||
|
|
||
| multiply(number) { | ||
| return this.result *= number; | ||
| }, | ||
|
|
||
| divide(number) { | ||
| return this.result /= number; | ||
| }, | ||
|
|
||
| reset() { | ||
| this.result = 0; | ||
|
|
||
| return this; | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those returns are unnecessary 'cause we use those functions as callbacks and don't really expect them to return anything, so I think we can remove those returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, don't mind that I also selected reset here; ignore this comment about reset.
src/makeCalculator.js
Outdated
| }, | ||
|
|
||
| divide(number) { | ||
| return this.result /= number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that raw divide could be harmful and break all calculations if number is zero. We would probably get infinity, and that's maybe not an expected behavior for this calculator. Could you check if the number is zero and maybe throw an error or reset the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 90be0ae
src/makeCalculator.js
Outdated
| }, | ||
|
|
||
| operate(callback, number) { | ||
| this.result = callback.call(this, number); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here we don't need this assigning, we already assign value in the called callbacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 90be0ae
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
I am approving your submission. You've done an excellent job implementing the makeCalculator function. All requirements have been met, including the creation of all necessary methods and the correct implementation of method chaining for operate and reset. Your code is clean, well-structured, and adheres to all the style guidelines. Great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.