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.

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

An alternate solution for pairs_with_given_sum #11415

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PavanVenkataNagaManoj17
Copy link

@PavanVenkataNagaManoj17 PavanVenkataNagaManoj17 commented May 24, 2024

Describe your change:

implemented a very simple alternative solution instead of using the itertools package

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
  • 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 include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

@algorithms-keeper algorithms-keeper bot added tests are failing Do not merge until tests pass and removed tests are failing Do not merge until tests pass labels May 24, 2024
anagrams.txt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this file for?

Comment on lines +26 to +27
# Without using the itertools Package we can do it too
def no_of_pairs(arr: list, req_sum: int) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is unnecessary. Instead, rename the function to clarify that it doesn't use itertools.

Comment on lines +28 to +30
# this is a non optimised code and easy to understand as i am a beginner in coding
# the code above is the best one to use as this function uses nested loops which is
# very much slower in execution
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add comments about your own thoughts about the code, just document what it does. If you want to clarify to the reader that your implementation is deliberately suboptimal, then please do so in the documentation. You should also be using docstrings for this rather than multiple single-line comments. Also make sure you add doctests in your documentation.

Comment on lines +27 to +36
def no_of_pairs(arr: list, req_sum: int) -> int:
# this is a non optimised code and easy to understand as i am a beginner in coding
# the code above is the best one to use as this function uses nested loops which is
# very much slower in execution
count_total = 0
for i in range(len(arr)):
for j in range(i + 1, len(arr)):
if (arr[i] + arr[j]) == req_sum:
count_total += 1
return count_total
Copy link
Contributor

Choose a reason for hiding this comment

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

@imSanko This entirely rewrites OP's implementation. OP said that the implementation was deliberately suboptimal to demonstrate a simple but naive solution.

count_total = 0
for i in range(len(arr)):
for j in range(i + 1, len(arr)):
if (arr[i] + arr[j]) == req_sum:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (arr[i] + arr[j]) == req_sum:
if arr[i] + arr[j] == req_sum:

The parentheses are unnecessary

@@ -23,6 +23,19 @@ def pairs_with_sum(arr: list, req_sum: int) -> int:
return len([1 for a, b in combinations(arr, 2) if a + b == req_sum])


# Without using the itertools Package we can do it too
def no_of_pairs(arr: list, req_sum: int) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def no_of_pairs(arr: list, req_sum: int) -> int:
def no_of_pairs(arr: list[int], req_sum: int) -> int:

Please specify the type of the list's contents

@tianyizheng02 tianyizheng02 added awaiting changes A maintainer has requested changes to this PR require proper documentation Requested to write the documentation properly require descriptive names This PR needs descriptive function and/or variable names labels Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes A maintainer has requested changes to this PR require descriptive names This PR needs descriptive function and/or variable names require proper documentation Requested to write the documentation properly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants