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

mergesort(in python) #77

Merged
merged 4 commits into from
Apr 3, 2024
Merged

mergesort(in python) #77

merged 4 commits into from
Apr 3, 2024

Conversation

abhirai7
Copy link
Collaborator

@abhirai7 abhirai7 commented Apr 3, 2024

No description provided.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @abhirai7 - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

  • Incorrect variable used for initializing 'k'. (link)
Here's what I looked at during the review
  • 🔴 General issues: 1 blocking issue, 2 other issues
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment on lines +5 to +6
left = [0] * (n1)
R = [0] * (n2)
Copy link

Choose a reason for hiding this comment

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

issue (code_clarification): Variable shadowing may lead to confusion.

The variable 'left' is used both as a parameter to the merge function and as a local variable within it. Consider renaming the local variable to avoid confusion and potential bugs.


i = 0
j = 0
k = left
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Incorrect variable used for initializing 'k'.

The variable 'k' is intended to be an index for the main array 'arr' but is mistakenly initialized with 'left', which is now a list. This should be initialized with the 'left' parameter's value instead.

arr = [1, 54, 343, 523]
merge_sort(arr, 0, len(arr) - 1)

print(arr)
Copy link

Choose a reason for hiding this comment

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

suggestion (code_refinement): Consider removing or commenting out debug prints before finalizing.

While useful during development, direct print statements like this can clutter output in production or when the code is used as part of a larger application.

@rtk-rnjn rtk-rnjn merged commit e8ce98d into rtk-rnjn:main Apr 3, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants