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

doc, fix: typo in merge sort and use vector instead of raw array #2841

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pipinstallaadit
Copy link

@pipinstallaadit pipinstallaadit commented Oct 15, 2024

  1. Typographical Error in Doxygen Comment

In the Doxygen comment at the beginning of the file, "Merege Sort" should be corrected to "Merge Sort".

  1. Merging Logic

In the merge function, the merging logic can lead to out-of-bounds access if both sub-arrays have been completely traversed. The condition in the while loop should ensure that you only access elements of L and R if they are within bounds:

  1. Memory Management

Using new and delete[] for the temporary arrays (L and R) is fine, but you can also use std::vector from the C++ Standard Library to simplify memory management:

  1. Input Validation

You should consider adding input validation when reading the number of elements and the actual elements to avoid undefined behavior.

  1. Main Function Improvements

You can also enhance the main function by encapsulating the input logic in a separate function, improving readability.

Description of Change

Checklist

  • [ x] Added description of change
  • [ x] Added file name matches File name guidelines
  • [ x] Added tests and example, test must pass
  • Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • Search previous suggestions before making a new one, as yours may be a duplicate.
  • I acknowledge that all my contributions will be made under the project's license.

Notes:

1. Typographical Error in Doxygen Comment

In the Doxygen comment at the beginning of the file, "Merege Sort" should be corrected to "Merge Sort".

2. Merging Logic

In the merge function, the merging logic can lead to out-of-bounds access if both sub-arrays have been completely traversed. The condition in the while loop should ensure that you only access elements of L and R if they are within bounds:

3. Memory Management

Using new and delete[] for the temporary arrays (L and R) is fine, but you can also use std::vector from the C++ Standard Library to simplify memory management:

4. Input Validation

You should consider adding input validation when reading the number of elements and the actual elements to avoid undefined behavior.

5. Main Function Improvements

You can also enhance the main function by encapsulating the input logic in a separate function, improving readability.
@realstealthninja realstealthninja changed the title Couple of issues and areas for improvement. doc, fix: typo in merge sort and use vector instead of raw array Oct 15, 2024
Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for this

@realstealthninja realstealthninja added approved Approved; waiting for merge hacktoberfest-accepted Accepted to be counted towards Hacktoberfest labels Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved; waiting for merge hacktoberfest-accepted Accepted to be counted towards Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants