HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaMinor

Merge sort efficiency

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sortefficiencymerge

Problem

I've written this Java code to implement merge sort:

import java.util.*;

public class MergeSort {

    int[] a;

    MergeSort() {
        Scanner scan = new Scanner(System.in);
        System.out.println("Enter the number of items: ");
        a = new int[scan.nextInt()];

        for (int i = 0; i = right[r]) {
                result[i] = right[r];
                r++;
            } else if (left[l] < right[r]) {
                result[i] = left[l];
                l++;
            }
        }

        if (l == left.length) {
            for (; r < right.length; ++r, ++i) {
                result[i] = right[r];
            }
        } else if (r == right.length) {
            for (; l < left.length; ++l, ++i) {
                result[i] = left[l];
            }
        } 
        return result;
    } 

    public static void main(String[] args) {
        MergeSort ms = new MergeSort();
        int[] result = ms.mergeSort2(0, ms.a.length - 1);
        System.out.println();
        for (int i = 0; i < result.length; ++i) {
            System.out.print(result[i] + " ");
        }
    }
}


Every time I want to return left and right to merge into result, I've to allocate result again (same for cases where length == 1). How can I avoid this allocation during recursion?

Solution

You can use a temporary array, see for example here or here.

As this would change quite a lot, I'm not going to rewrite your code to match those examples.

But I do have some additional comments which might improve the readability and ease of use of your code:

Constructor

A constructor should only construct an object. Your constructor also gathers user input. This makes it extremely hard to test your class.

Just let MergeSort accept an array, which you created in a ConsoleInput class (or main).

Or pass the array directly to the sort method and just use an empty constructor.

Merging

Your merging code is a bit confusing, because you are using for loops when while would be a better fit:

int l = 0;
    int r = 0;
    int arrayPosition = 0;
    while (l = right[r]) {
            result[arrayPosition] = right[r];
            r++;
        } else {
            result[arrayPosition] = left[l];
            l++;
        }
        arrayPosition++;
    }

    // copy rest
    while (l < left.length) {
        result[arrayPosition] = left[l];
        l++;
        arrayPosition++;
    }
    while (r < right.length) {
        result[arrayPosition] = right[r];
        r++;
        arrayPosition++;
    }


With this code, you can probably also better see the similarities to the code that uses a temporary array.

And to copy the rest, you could also use arraycopy, which is probably faster:

if (l == left.length) {
        System.arraycopy(right, r, result, arrayPosition, right.length - r);
    } else if (r == right.length) {
        System.arraycopy(left, l, result, arrayPosition, left.length - l);
    }


Other

  • naming: mergeSort2 why 2? And just write beginning instead of beg, it's not that long.



  • private: fields should be private (unless there is a good reason to make them public), same with methods.



  • create a merge method, it makes your code easier to read.



  • the caller shouldn't need to know with what parameters to call mergeSort2. I would create a public method without arguments (or which accepts the array) which then calls mergeSort2 with the correct parameters.

Code Snippets

int l = 0;
    int r = 0;
    int arrayPosition = 0;
    while (l < left.length && r < right.length) {
        if (left[l] >= right[r]) {
            result[arrayPosition] = right[r];
            r++;
        } else {
            result[arrayPosition] = left[l];
            l++;
        }
        arrayPosition++;
    }

    // copy rest
    while (l < left.length) {
        result[arrayPosition] = left[l];
        l++;
        arrayPosition++;
    }
    while (r < right.length) {
        result[arrayPosition] = right[r];
        r++;
        arrayPosition++;
    }
if (l == left.length) {
        System.arraycopy(right, r, result, arrayPosition, right.length - r);
    } else if (r == right.length) {
        System.arraycopy(left, l, result, arrayPosition, left.length - l);
    }

Context

StackExchange Code Review Q#62880, answer score: 5

Revisions (0)

No revisions yet.