-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
add solution #2328
base: master
Are you sure you want to change the base?
add solution #2328
Conversation
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.
The implementation of the makeCalculator
function generally meets the requirements, but there are a few issues that need to be addressed regarding code style and code knowledge.
src/makeCalculator.js
Outdated
operate(action, number) { | ||
this.result = action(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.
Consider using the arithmetic operators with the assignment operator for better code style. For example, this.result += number
when the action is addition. Since operate
is a generic method, you may need to adjust the add
, subtract
, multiply
, divide
methods to work with the current value of result
directly.
src/makeCalculator.js
Outdated
add: (a, b) => a + b, | ||
subtract: (a, b) => a - b, | ||
multiply: (a, b) => a * b, | ||
divide: (a, b) => a / b, |
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.
Arrow functions should not be used for methods that will be using this
from their containing object, because arrow functions do not have their own this
context; they inherit this
from the parent scope at the time they are defined. In this case, since the methods are using this.result
in the operate
method, they should be defined using the shorthand method syntax without the arrow function.
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.
Nice start! Let's fix and improve your work a little bit)
multiply(num) { | ||
this.result *= num; | ||
}, | ||
divide(num) { |
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.
Consider case with division to 0 and throw error in such case
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.
lgtm
No description provided.