Skip to content

Fix coin change#2571

Merged
poyea merged 12 commits into
TheAlgorithms:masterfrom
himadriganguly:patch-1
Oct 23, 2020
Merged

Fix coin change#2571
poyea merged 12 commits into
TheAlgorithms:masterfrom
himadriganguly:patch-1

Conversation

@himadriganguly
Copy link
Copy Markdown
Contributor

@himadriganguly himadriganguly commented Oct 1, 2020 •
edited
Loading

Describe your change:

Removed unused variable and modified code for coin_chainge algo.

  • 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}.

Kush1101
Kush1101 reviewed Oct 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Kush1101 Kush1101 left a comment

Choose a reason for hiding this comment

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

Is it really a useful contribution?

@poyea poyea added invalid and removed invalid labels Oct 1, 2020
poyea
poyea requested changes Oct 1, 2020
View reviewed changes
Comment thread dynamic_programming/coin_change.py Outdated
@poyea poyea added the tests are failing Do not merge until tests pass label Oct 1, 2020
@himadriganguly himadriganguly requested a review from poyea October 1, 2020 16:12
poyea
poyea reviewed Oct 1, 2020
View reviewed changes
Comment thread dynamic_programming/coin_change.py Outdated
Comment on lines +22 to +29
>>> dp_count([1, 2, 3], -5)
0
>>> dp_count([4, 5, 6], -4)
0
>>> dp_count([2, 5, 3, 6], -10)
0
>>> dp_count([4], -4)
0
Copy link
Copy Markdown
Member

@poyea poyea Oct 1, 2020

Choose a reason for hiding this comment

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

Suggested change
>>> dp_count([1, 2, 3], -5)
0
>>> dp_count([4, 5, 6], -4)
0
>>> dp_count([2, 5, 3, 6], -10)
0
>>> dp_count([4], -4)
0
>>> dp_count([1, 2, 3], -5)
0

I think 1 test case for this is enough.

Copy link
Copy Markdown
Contributor Author

@himadriganguly himadriganguly Oct 1, 2020

Choose a reason for hiding this comment

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

@poyea You said to add some more test cases in the previous comment?

Copy link
Copy Markdown
Member

@poyea poyea Oct 5, 2020

Choose a reason for hiding this comment

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

I mean to test the < 0 line, one test case is enough. You may add some larger test cases too to demonstrate the correctness.

@poyea poyea removed the tests are failing Do not merge until tests pass label Oct 1, 2020
@poyea
Copy link
Copy Markdown
Member

poyea commented Oct 1, 2020

Can you change the title of the pull request to something like fix coin change?

@poyea poyea mentioned this pull request Oct 1, 2020
Closed
@himadriganguly himadriganguly changed the title Patch 1 Fix coin change Oct 1, 2020
@himadriganguly
Copy link
Copy Markdown
Contributor Author

himadriganguly commented Oct 5, 2020

@poyea done the changes have a look.

@poyea poyea merged commit 46af42d into TheAlgorithms:master Oct 23, 2020
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
@himadriganguly @ruppysuppy @stokhos
Fix coin change (TheAlgorithms#2571)
d5c675b 
* Removed unused variable m.

* Doctests are modified to match functions.

* Added condition for negative values.

* Fixed white-space around operator.

* Fixed W293 blank line contains white-space error.

* Update dynamic_programming/coin_change.py

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

* Fixed error in code.

* Fixed whited spacing.

* Fixed PEP8 error.

* Added more test cases for coin change problem.

* Removed extra test for negetive value.

Co-authored-by: Tapajyoti Bose <44058757+ruppysuppy@users.noreply.github.com>
peRFectBeliever pushed a commit to peRFectBeliever/Python that referenced this pull request Apr 1, 2021
@himadriganguly @ruppysuppy @peRFectBeliever
Fix coin change (TheAlgorithms#2571)
7bbd8a3 
* Removed unused variable m.

* Doctests are modified to match functions.

* Added condition for negative values.

* Fixed white-space around operator.

* Fixed W293 blank line contains white-space error.

* Update dynamic_programming/coin_change.py

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

* Fixed error in code.

* Fixed whited spacing.

* Fixed PEP8 error.

* Added more test cases for coin change problem.

* Removed extra test for negetive value.

Co-authored-by: Tapajyoti Bose <44058757+ruppysuppy@users.noreply.github.com>
Panquesito7 pushed a commit to Panquesito7/Python that referenced this pull request May 13, 2021
@himadriganguly @ruppysuppy
Fix coin change (TheAlgorithms#2571)
fe5a044 
* Removed unused variable m.

* Doctests are modified to match functions.

* Added condition for negative values.

* Fixed white-space around operator.

* Fixed W293 blank line contains white-space error.

* Update dynamic_programming/coin_change.py

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

* Fixed error in code.

* Fixed whited spacing.

* Fixed PEP8 error.

* Added more test cases for coin change problem.

* Removed extra test for negetive value.

Co-authored-by: Tapajyoti Bose <44058757+ruppysuppy@users.noreply.github.com>
shermanhui pushed a commit to shermanhui/Python that referenced this pull request Oct 22, 2021
@himadriganguly @ruppysuppy @shermanhui
Fix coin change (TheAlgorithms#2571)
69a9ac7 
* Removed unused variable m.

* Doctests are modified to match functions.

* Added condition for negative values.

* Fixed white-space around operator.

* Fixed W293 blank line contains white-space error.

* Update dynamic_programming/coin_change.py

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

* Fixed error in code.

* Fixed whited spacing.

* Fixed PEP8 error.

* Added more test cases for coin change problem.

* Removed extra test for negetive value.

Co-authored-by: Tapajyoti Bose <44058757+ruppysuppy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kush1101 Kush1101 Kush1101 left review comments

@poyea poyea poyea approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@himadriganguly @poyea @Kush1101