4
\$\begingroup\$

Could you provide feedback for this code? For arrays of length 2, is it more efficient not to use a sorting algorithm?

package insertion.sort;

import java.util.Arrays;

public class InsertionSort {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        int[] testArr = {-10, -5, 100, 51, 6, 50};
        insertionSort(testArr);
        System.out.println(Arrays.toString(testArr));
    }

    public static void insertionSort(int[] arr) {
        for(int i = 1; i < arr.length; i++) {
            int temp = arr[i];
            int j;
            for(j = i; j > 0 && arr[j-1] > temp; j--)
                arr[j] = arr[j-1];
            arr[j] = temp;
        }
    }

}

By the way, I've noticed "quicksort" is spelt all as one word, but "insertion sort" is spelt as two. Is that right? Would anyone ever say "quicksort sort"?

\$\endgroup\$
  • 1
    \$\begingroup\$ You could consider finding out about the grammar question on English Language & Usage. It doesn't really relate to the code in any way. \$\endgroup\$ – Jamal Jul 11 '14 at 17:34
2
\$\begingroup\$

Minor issues

  • The word "insertion" in the method name insertionSort is redundant: it's already in the class name

  • The Javadoc for the main method is pointless, it would be better to remove

  • Perhaps you did it this way for the sake of an example, but the main method would be better in another class, not InsertionSort itself.

Generalizing

It might be good to generalize the class so that it can sort anything, not only integers. Perhaps something like this:

public class InsertionSort<T extends Comparable<T>> {    
    public void sort(T[] arr) {
        for (int i = 1; i < arr.length; i++) {
            T temp = arr[i];
            int j;
            for (j = i; j > 0 && arr[j - 1].compareTo(temp) > 0; j--) {
                arr[j] = arr[j - 1];
            }
            arr[j] = temp;
        }
    }
}

class InsertionSortDemo {
    public static void main(String[] args) {
        Integer[] ints = {-10, -5, 100, 51, 6, 50};
        new InsertionSort<Integer>().sort(ints);
        System.out.println(Arrays.toString(ints));

        String[] strings = {"hello", "there", "my", "friend"};
        new InsertionSort<String>().sort(strings);
        System.out.println(Arrays.toString(strings));
    }
}
| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

As insertion sort is an O(n2) algorithm, there's not much point to optimizing it. For any input that is large enough for you to care about the performance, you would want to pick a sorting algorithm with better bounds. Quicksort, for example, is usually closer to O(n log n).

That said, I'll point out some style issues:

  • You should never omit optional braces, as you've done for the inner for-loop. Think of it this way: anytime you omit braces, you're a contributing factor to a future coding accident. (Apple learned this lesson the hard way; their new language requires braces.)
  • Variables can usually be named better than "temp". Renaming temp to elementToInsert, for example, would make your code self-documenting.
| improve this answer | |
\$\endgroup\$
-1
\$\begingroup\$

I do not see any performance issues. Stylistically, a No Raw Loops rule mandates factoring out an inner loop into a method (shift_right or something like that).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I don't get raw loops. All those examples are for C++. \$\endgroup\$ – Celeritas Jul 11 '14 at 19:04
  • \$\begingroup\$ -1 for linking to google search \$\endgroup\$ – Celeritas Jul 11 '14 at 20:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.