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

Update QuickSort.java #5089

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

Conversation

manishraj27
Copy link

I've made the following changes:
1.Renamed doSort to quickSort for clarity.
2.Renamed randomPartition to partitionWithRandomPivot. 3.Added inline comments for clarity.
4.Reused existing utility methods from SortUtils.
5.Ensured error handling by checking for edge cases like null arrays. 6.Maintained the core logic of the QuickSort algorithm while optimizing readability and maintainability.

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized it.
  • All filenames are in PascalCase.
  • All functions and variable names follow Java naming conventions.
  • All new algorithms have a URL in their comments that points to Wikipedia or other similar explanations.
  • All new code is formatted with clang-format -i --style=file path/to/your/file.java

I've made the following changes:
1.Renamed doSort to quickSort for clarity.
2.Renamed randomPartition to partitionWithRandomPivot.
3.Added inline comments for clarity.
4.Reused existing utility methods from SortUtils.
5.Ensured error handling by checking for edge cases like null arrays.
6.Maintained the core logic of the QuickSort algorithm while optimizing readability and maintainability.
*/
@Override
public <T extends Comparable<T>> T[] sort(T[] array) {
doSort(array, 0, array.length - 1);
quickSort(array, 0, array.length - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Error Handling: You should add a check to handle cases where the array is null or has only one element, returning the array directly in such cases.

Suggested change
quickSort(array, 0, array.length - 1);
if (array == null || array.length <= 1) {
return array;
}
quickSort(array, 0, array.length - 1);

@TruongNhanNguyen
Copy link
Contributor

TruongNhanNguyen commented Apr 16, 2024

Random Pivot Selection: In your implementation, the pivot selection logic should be more intuitive and clear. The pivot is always chosen from the rightmost element of the array.

* @author Podshivalov Nikita (https://github.com/nikitap492)
* Implementation of the QuickSort algorithm.
*
* @author Manish Raj (https://github.com/manishraj27)
* @see SortAlgorithm
*/
class QuickSort implements SortAlgorithm {

/**
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
/**
private static final int RANDOM_BOUND = 1;
/**

*/
private static <T extends Comparable<T>> int randomPartition(T[] array, int left, int right) {
private static <T extends Comparable<T>> int partitionWithRandomPivot(T[] array, int left, int right) {
int randomIndex = left + (int) (Math.random() * (right - left + 1));
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
int randomIndex = left + (int) (Math.random() * (right - left + 1));
int randomIndex = left + (int) (Math.random() * (right - left + RANDOM_BOUND));

@TruongNhanNguyen
Copy link
Contributor

TruongNhanNguyen commented Apr 16, 2024

Partitioning Logic: Simplified the partitioning logic by using a single loop instead of nested while loops, which makes it easier to understand.

What do you think about this suggestion:

    /**
     * Partitions the array around a pivot element.
     *
     * @param array The array to be partitioned.
     * @param left  The leftmost index of the subarray.
     * @param right The rightmost index of the subarray.
     * @return The index of the pivot element after partitioning.
     */
    private static <T extends Comparable<T>> int partition(T[] array, int left, int right) {
        T pivot = array[right];
        int i = left - 1;

        for (int j = left; j < right; j++) {
            if (array[j].compareTo(pivot) <= 0) {
                i++;
                swap(array, i, j);
            }
        }

        swap(array, i + 1, right);
        return i + 1;
    }
}

Copy link

This pull request 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 contribution!

@github-actions github-actions bot added the stale label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants