-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Refactor Maths/Factorial for more minimal side effects. #477
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
Conversation
@pomkarnath98 Why to don't merge this PR?
Factorial is already there in the list. So we can only merge if it is not present in the list. Hope you understand.
Thank you
Factorial is already there in the list. So we can only merge if it is not present in the list. Hope you understand.
Thank you
It's already but i'm refactor code to minimal side effects.
Ok, we will get back to you soon.
But you should mention the #{ISSUE_NO} that your code resolved. And if it's not there you could create one and then mention by stating that the code ia fixing that issue.
Fixed #503
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.
Hey @Eji4h, the refactoring looks good. There one thing that needs changing.
Move the (num > 0) if condition to the end.
It's a general practice to validate the input first and then calculate and return the solution if the input passes validation.
Welcome to JavaScript community
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare notFixes: #{$ISSUE_NO}.