Skip to main content
added 24 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

Good job in general. Just few comments.

  • All the algorithms are implemented on arrays. It is really an overkill; you don't need a random access to sort a collection.

  • I am not sure you need to spell out less, equals or greater. Isn't it what Comparable is for?

  • I am not sure that isSorted(Comparable[] a, int lo, int hi) needs to exist at all. It doesn't hurt to have it though.

  • mergeArraysmergeArrays()

    A naked loop usually represents an important algorithm. In this case a for (int k = low; k <= high; k++) loop is even attributed with comment; definitely it is copy(), which deserves a method of its own.

    Once a i > mid condition is encountered, there is no point to test it over and over again: break the main loop right away and let a copy() method do the rest. Same goes for j > high.

A naked loop usually represents an important algorithm. In this case a for (int k = low; k <= high; k++) loop is even attributed with comment; definitely it is copy(), which deserves a method of its own.

Once a i > mid condition is encountered, there is no point to test it over and over again: break the main loop right away and let a copy() method do the rest. Same goes for j > high.

  • quickPartition

    quickPartition()

    Unnecessary comments sometimes create very hard to understand problems. It took me a while to realize that greater at line 153 is an unfortunate copy-paste. Yet another reason to convert a naked loop into a method.

Unnecessary comments sometimes create very hard to understand problems. It took me a while to realize that greater at line 153 is an unfortunate copy-paste. Yet another reason to convert a naked loop into a method.

  • insertionSort

    insertionSort()

    Same naked loop problem. An inner loop is identical to one of the quickPartition's loops.

Same naked loop problem. An inner loop is identical to one of the quickPartition's loops.

  • selectionSort

    selectionSort()

    See an insertionSort comment.

See an insertionSort comment

  • shellSort

    shellSort()

    Did I mention naked loops?

Did I mention naked loops?

  • main

    main()

    Do something with all those Stopwatches! You already provided arrayCopy - why not use it? The Java experts will comment on Java specifics (final especially).

Do something with all those Stopwatches!

PS: you already provided arrayCopy - why not use it?

PPS: the Java experts will comment on Java specifics (final especially).

Good job in general. Just few comments.

  • All the algorithms are implemented on arrays. It is really an overkill; you don't need a random access to sort a collection.

  • I am not sure you need to spell out less, equals or greater. Isn't it what Comparable is for?

  • I am not sure that isSorted(Comparable[] a, int lo, int hi) needs to exist at all. It doesn't hurt to have it though.

  • mergeArrays

A naked loop usually represents an important algorithm. In this case a for (int k = low; k <= high; k++) loop is even attributed with comment; definitely it is copy(), which deserves a method of its own.

Once a i > mid condition is encountered, there is no point to test it over and over again: break the main loop right away and let a copy() method do the rest. Same goes for j > high.

  • quickPartition

Unnecessary comments sometimes create very hard to understand problems. It took me a while to realize that greater at line 153 is an unfortunate copy-paste. Yet another reason to convert a naked loop into a method.

  • insertionSort

Same naked loop problem. An inner loop is identical to one of the quickPartition's loops.

  • selectionSort

See an insertionSort comment

  • shellSort

Did I mention naked loops?

  • main

Do something with all those Stopwatches!

PS: you already provided arrayCopy - why not use it?

PPS: the Java experts will comment on Java specifics (final especially).

Good job in general. Just few comments.

  • All the algorithms are implemented on arrays. It is really an overkill; you don't need a random access to sort a collection.

  • I am not sure you need to spell out less, equals or greater. Isn't it what Comparable is for?

  • I am not sure that isSorted(Comparable[] a, int lo, int hi) needs to exist at all. It doesn't hurt to have it though.

  • mergeArrays()

    A naked loop usually represents an important algorithm. In this case a for (int k = low; k <= high; k++) loop is even attributed with comment; definitely it is copy(), which deserves a method of its own.

    Once a i > mid condition is encountered, there is no point to test it over and over again: break the main loop right away and let a copy() method do the rest. Same goes for j > high.

  • quickPartition()

    Unnecessary comments sometimes create very hard to understand problems. It took me a while to realize that greater at line 153 is an unfortunate copy-paste. Yet another reason to convert a naked loop into a method.

  • insertionSort()

    Same naked loop problem. An inner loop is identical to one of the quickPartition's loops.

  • selectionSort()

    See an insertionSort comment.

  • shellSort()

    Did I mention naked loops?

  • main()

    Do something with all those Stopwatches! You already provided arrayCopy - why not use it? The Java experts will comment on Java specifics (final especially).

Source Link
vnp
  • 58.7k
  • 4
  • 55
  • 144

Good job in general. Just few comments.

  • All the algorithms are implemented on arrays. It is really an overkill; you don't need a random access to sort a collection.

  • I am not sure you need to spell out less, equals or greater. Isn't it what Comparable is for?

  • I am not sure that isSorted(Comparable[] a, int lo, int hi) needs to exist at all. It doesn't hurt to have it though.

  • mergeArrays

A naked loop usually represents an important algorithm. In this case a for (int k = low; k <= high; k++) loop is even attributed with comment; definitely it is copy(), which deserves a method of its own.

Once a i > mid condition is encountered, there is no point to test it over and over again: break the main loop right away and let a copy() method do the rest. Same goes for j > high.

  • quickPartition

Unnecessary comments sometimes create very hard to understand problems. It took me a while to realize that greater at line 153 is an unfortunate copy-paste. Yet another reason to convert a naked loop into a method.

  • insertionSort

Same naked loop problem. An inner loop is identical to one of the quickPartition's loops.

  • selectionSort

See an insertionSort comment

  • shellSort

Did I mention naked loops?

  • main

Do something with all those Stopwatches!

PS: you already provided arrayCopy - why not use it?

PPS: the Java experts will comment on Java specifics (final especially).