Skip to content

Conversation

@himadriganguly
Copy link
Contributor

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

Comment on lines 10 to 12
def dp_count(S, n):
"""
>>> dp_count([1, 2, 3], 3, 4)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def dp_count(S, n):
"""
>>> dp_count([1, 2, 3], 3, 4)
from typing import List, Optional
def coin_change(make_change_for: int, coin_types: List[int] = None) -> int:
"""
>>> coin_change(4, [1, 2, 3])

If the doctests are not modified to match the function then our automated tests will fail.

While we are here, let's modify the function to use self-documenting names for function and variables. Let's also add Python type hints.

Copy link
Member

@cclauss cclauss Jul 15, 2020

Choose a reason for hiding this comment

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

Before line 24 we could give coin_types a default value by adding the line coin_types = coin_types or (1, 5, 10, 25, 100)

This would allow a call such as coin_change(17).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the doctests.

Before line 24 we could give coin_types a default value by adding the line coin_types = coin_types or (1, 5, 10, 25, 100)
This would allow a call such as coin_change(17).

Didn't get this point can you please explain.

@cclauss
Copy link
Member

cclauss commented Jul 15, 2020

Please add a test where make_change_for = -5. How should the algorithm behave with that input?

@himadriganguly
Copy link
Contributor Author

Please add a test where make_change_for = -5. How should the algorithm behave with that input?

Added condition for negative values and the doctest.

@TravisBuddy
Copy link

Hey @himadriganguly,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 373c2f10-c6db-11ea-aa06-e17841301e13

@TravisBuddy
Copy link

Hey @himadriganguly,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 20a30200-c6dc-11ea-aa06-e17841301e13

Copy link
Member

@ruppysuppy ruppysuppy left a comment

Choose a reason for hiding this comment

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

The doctest is failing

Co-authored-by: Tapajyoti Bose <44058757+ruppysuppy@users.noreply.github.com>
@TravisBuddy
Copy link

Travis tests have failed

Hey @himadriganguly,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: e2aef8b0-ca9f-11ea-84bb-258f0278865c

@ruppysuppy
Copy link
Member

The doctests are failing, make sure that all the doctests are successful

@stale
Copy link

stale bot commented Aug 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Used to mark an issue or pull request stale. label Aug 21, 2020
@stale
Copy link

stale bot commented Aug 28, 2020

Please reopen this issue once you commit the changes requested or make improvements on the code. Thank you for your contributions.

@stale stale bot closed this Aug 28, 2020
@TheAlgorithms TheAlgorithms deleted a comment from Rohit-wed Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Used to mark an issue or pull request stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants