这个归并排序算法有什么问题？

Question

谁能告诉我这段代码有什么问题？这段代码是使用归并排序对数组中的一组元素进行排序。

#include<iostream>

void merge(int arr[], int left, int mid, int right){
    int left_ptr = left;
    int right_ptr = mid + 1;
    int size = right - left + 1;       
    int temp[size];
    int k = left;

    while (left_ptr <= mid && right_ptr <= right)
    {
        if(arr[left_ptr] <= arr[right_ptr]){
            temp[k] = arr[left_ptr];
            left_ptr++;
            k++;
        }
        else{
            temp[k] = arr[right_ptr];
            right_ptr++;
            k++;
        }
        
    }

    while (left_ptr <= mid)
    {
        temp[k] = arr[left_ptr];
        left_ptr++;
        k++;
    }

    while (right_ptr <= right)
    {
        temp[k] = arr[right_ptr];
        right_ptr++;
        k++;
    }
    
    for (int i = left_ptr; i < k; i++)
    {
        arr[i] = temp[i];
    }
    
}

void mergeSort(int arr[], int left, int right){
    int mid;
    if (left < right)
    {
        mid = (right + left)/2;
        mergeSort(arr, left, mid);
        mergeSort(arr, mid + 1, right);
        merge(arr, left, mid, right);
    }
    
}
int main(){
    int arr[] = {45,8,9,7,4,58,2,34,2,58}; 
    std::cout << arr << std::endl; 
    int size = sizeof(arr)/sizeof(int);
    mergeSort(arr, 0, size - 1);
    for (int i = 0; i < size; i++)
    {
        std::cout << arr[i] << "    ";
    }

    std::cout << std::endl;
    
}

我用许多在线代码仔细检查了它，我没有发现任何错误......你认为哪里出了问题？我尝试使用就地数组（类似于快速排序）来实现这一点。

Answer 1

这里列出了您的代码有问题的地方。我在这里广泛使用“错误”。对于每一段代码，我的主要批评是基于样式而不是“正确性”，其中针对的样式是使正确性更容易被发现的样式。

在此过程中，其中一种风格批评导致发现了看起来像错误的地方。

void merge(int arr[], int left, int mid, int right){

1. 您正在使用int 来引用数组中的偏移量。

2. 您正在使用int[] 参数，这是int* arr 的传统C 语法。请改用std::span 之类的东西。

继续：

 int left_ptr = left;

如果您的目标是保留原始参数并处理副本，请将原始参数设置为 const，这样就不必证明它们在函数体中没有发生变异。

int right_ptr = mid + 1;

您有名为 _ptr 的变量不是指针。

int size = right - left + 1;

您似乎没有使用半开区间。使用并学习使用半开区间。它们在 C++ 中是传统的，并且确实摆脱了许多栅栏后更正代码。

int temp[size];

这不符合 C++ 标准。实际上，即使在支持这一点的编译器上，许多 C++ 实现的堆栈也比您可能想要排序的数组的内存小得多。这会导致您的代码炸毁它的堆栈。

正确性比性能更重要。在堆栈上创建动态大小的对象会导致程序出现未定义的行为或在其他合理的输入上崩溃。

int k = left;

这个变量没有描述它的作用。

while (left_ptr <= mid && right_ptr <= right)
while (left_ptr <= mid)
while (right_ptr <= right)

这些循环中有很多代码重复。

DRY - 不要重复自己。在这里，如果任何一个重复中存在错误，如果您 DRY，则该错误将在所有用途中使用并且更容易发现。这里有很多 DRY 方法（lambdas、辅助函数、稍微复杂的分支和一个循环）；使用其中之一。

for (int i = left_ptr; i < k; i++)
{
    arr[i] = temp[i];
}

看起来像手动标准副本？看起来它也有一个错误，因为当然手动重新实现 std 副本意味着你做错了。

void mergeSort(int arr[], int left, int right){

同样，传统 C 风格的数组传递。

    int mid;

无需初始化就无需声明它。将声明尽可能地移到它们的第一次使用点，并尽快让它们超出范围。

    if (left < right)
    {
        mid = (right + left)/2;

制作这个int mid =。

        mergeSort(arr, left, mid);
        mergeSort(arr, mid + 1, right);

一个例子说明封闭的间隔如何让你不得不做烦人的围栏张贴。

        merge(arr, left, mid, right);

mergeSort(arr, 0, size - 1);

这里是另一个栅栏 +/- 1。

Answer 2

我在代码中看到两个可能的错误：

在merge 中声明int temp[size]; 无效，因为size 不是常量。您需要动态分配数组。

其次，在merge 函数的最后一段（for 循环）中，初始化i = left_ptr。但是，left_ptr 在此之前设置为等于 mid。我相信你确实想初始化i = left。

编辑：刚刚注意到：temp 不一定必须从 arr 的开头开始。我的意思是，temp 的每个元素都映射到arr 的特定元素，但是您的代码假设在几个地方，temp[0] 映射到arr[0]，这是不正确的（temp[0]实际上映射到arr[right])。有两种方法可以解决这个问题。

您可以修复基于此假设的部分。在最终的for 循环中使用arr[i + right] = temp[i] 代替arr[i] = temp[i]，并将k 初始化为零。

第二个选项是，而不是在每一个对merge 的调用中创建和删除temp，而是将其创建为与arr 相同的大小，并完全保留它算法的执行（可以通过在算法之外创建它并将其传递给merge 或MergeSort 的每个调用来完成）这样，相等的偏移假设实际上是正确的。