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.

Sign up for GitHub

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

Jump to bottom

Add Krishnamurty Number to Math algorithms #728

Merged
merged 3 commits into from
Oct 23, 2020
Merged

Add Krishnamurty Number to Math algorithms #728

merged 3 commits into from
Oct 23, 2020

Conversation

Nanak360
Copy link
Contributor

I have added the Krishnamurthy Number checker algorithm. This is a common algorithm that is pretty famous here in India especially from a beginner's perspective.
Talking of Krishnamurthy number, it's a number whose sum total of the factorials of each digit is equal to the number itself.

hacktoberfest-accepted

@ericklarac
Copy link
Collaborator

Could you add some tests under the test folder?

@Nanak360
Copy link
Contributor Author

Yes, I'll surely do that @ericklarac.
I would also like to add that I am new to opensource, motivated by Hacktoberfest. Would you please help me to make this PR count as one of my Hacktoberfest PRs?
If you'll just add a "hacktoberfest-accepted" label to this Issue, it'll mean a lot to me.

ericklarac
ericklarac reviewed Oct 19, 2020
View reviewed changes
Copy link
Collaborator

@ericklarac ericklarac left a comment

Choose a reason for hiding this comment

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

Add some test and add your implementation to the readme, you can use this merged PR as a guide

algorithms/maths/krishnamurthy_number.py Outdated
return fact


def checkKrishnamurthy(n):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use snake case

algorithms/maths/krishnamurthy_number.py Outdated


def checkKrishnamurthy(n):
sumOfDigits = 0 # will hold sum of FACTORIAL of digits
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, se snake case for the variables

@Nanak360
Copy link
Contributor Author

For some reason, the tests failed.
I'm not understanding why. Would someone help me out?

@ericklarac
Copy link
Collaborator

For some reason, the tests failed.
I'm not understanding why. Would someone help me out?

Traceback (most recent call last):
File "/home/travis/build/keon/algorithms/tests/test_maths.py", line 329, in test_factorial
self.assertEqual(637816310, factorial(34521, 10**9 + 7))
TypeError: factorial() takes 1 positional argument but 2 were given.

The factorial function was previously defined in other file. You are exporting your function, and it is overriding the other function.

A workaround could be to rename the factorial function on krishnamurthy_number.py, or try only to export krishnamurthy_number on the algorithms/maths/__init__.py file.

@Nanak360
Copy link
Contributor Author

Okay, I am doing the changes.
Could you please a "hacktoberfest-accepted" label to this PR?

@ericklarac ericklarac merged commit 30512d9 into keon:master Oct 23, 2020
@Nanak360
Copy link
Contributor Author

Yaaaeee...!!!!
It's finally merged! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericklarac ericklarac ericklarac left review comments

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Nanak360 @ericklarac