MaxHeap implementation

I'd like this to be reviewed:

```
public class MaxHeap> {
private E[] heap;
private int capacity; // maximum size of heap
private int numberOfNodes; // number of nodes in current heap

/**
* Create a new MaxHeap object.
*
* @param heap
* @param capacity
* @param numberOfNodes
*/
public MaxHeap(E[] heap, int capacity, int numberOfNodes) {
this.heap = heap;
this.capacity = capacity;
this.numberOfNodes = numberOfNodes;

this.buildHeap();
}

/**
* Put all nodes within the max heap in the correct position.
*/
void buildHeap() {
for (int i = (this.numberOfNodes / 2 - 1); i >= 0; i--) {
this.correctNodeIndexByShifting(i);
}
}

/**
* Insert a new node at the current position within the max-heap.
*
* @param nodeValue
* The node to be inserted.
*/
public void insert(E nodeValue) {
if (this.capacity 0)) {
this.swap(currentNodePosition,
this.getParentIndex(currentNodePosition));
currentNodePosition = this.getParentIndex(currentNodePosition);
}
}

/**
* Remove the node at arrayIndex within the MaxHeap and return the node
* value that the removed node is replaced with.
*
* @param arrayIndex
* Index of the node within the array based max-heap to be
* removed.
* @return The element that was removed.
*/
public E remove(int arrayIndex) {
int changingArrayIndex = arrayIndex;
if ((changingArrayIndex = this.numberOfNodes)) {
throw new IllegalArgumentException("In method remove of class "
+ "MaxHeap the input node postion to be removed is invalid");
}

// if the most bottom right node is being removed there is no work to be
// done
if (changingArrayIndex == (this.numberOfNodes - 1)) {
this.numberOfNodes--;
} else {
// swap node to be removed with most bottom right n

I haven't checked the details of the algorithm, just some general feedback on the code, API, etc:

-
It's usually a good practice to make a copy of mutable input parameters. (E[] heap in this case.) It prohibits malicious clients to modify the heap's internal structure or it could save you from a few hours of debugging. (Effective Java, 2nd Edition, Item 39: Make defensive copies when needed)

Copying the input array would make the capacity field redundant. You could use heap.length instead of it.

-
The constructor should validate its input parameters. Currently it's too easy to call it with a wrong size array:

String[] heap = {"aa", "ac", "bb"};
int capacity = 4;
int numberOfNodes = 4;
MaxHeap maxHeap =
    new MaxHeap(heap, capacity, numberOfNodes);

If you call a maxHeap.insert("ab") after that you get a mysterious ArrayIndexOutOfBoundsException. (Effective Java, Second Edition, Item 38: Check parameters for validity) (Copying the input array to a properly sized array could solve this issue too.)

-
It's easy to misunderstood the parameter of remove(). Currently it's an index. A client easily think that the following code removes 20 from the heap:

final Integer[] initialData = {10, 30, 20, 40};
int capacity = 4;
int numberOfNodes = 3;
MaxHeap heap =
    new MaxHeap(initialData, capacity, numberOfNodes);
heap.remove(20);

Actually it throws an exception, since there aren't 20 items in the heap.

Note that there isn't any other method which returns an index so how could a client figure out a valid parameter of the remove method? A remove(E item) method would be better.

-
Implementation of printMaxHeapArray could be replaced with return Arrays.toString(heap); if output format is not bound. (Output of Arrays.toString() is a slightly different: [cc, bb, aa, ab]). Otherwise, I'd use a more compact foreach loop:

for (final E item: heap) {
    maxHeapArray.append(item + " ");
}

Checking it again I guess i

-
Java already has a toString() method. If you use printMaxHeapArray only for debugging or logging purposes and clients don't rely on its exact output you should implement (override) toString() instead. toString() is more convenient for most developers. (Effective Java, 2nd Edition, Item 10: Always override toString)

-
Comments like this are unnecessary:

/**
 * Create a new MaxHeap object.
 * 
 * @param heap
 * @param capacity
 * @param numberOfNodes
 */

They say nothing more than the code already does, it's rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

-
Instead of comments like this:

* @param arrayIndex
 *            Index of parent node.

rename the parameter to parentIndex or parentNodeIndex. It would help readers, make the code readable and you could get rid of the comment. The same is true for getParentIndex(final int arrayIndex) (childIndex).

-
boolean isLeafNode(final int arrayIndex) returns false when the given index is higher than the size of the array. I guess calling the method with a definitely invalid index is a bug in the client code. Crash early, throw an IllegalArgumentException (as the getLeftChildIndex() method does). See: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.

-
The else keyword is unnecessary here:

if (arrayIndex1  this.numberOfNodes) {
    throw new IllegalArgumentException("In method swap of class "
            + "MaxHeap the input arrayIndex1 is not a valid node position");
} else if (arrayIndex2  this.numberOfNodes) {
    throw new IllegalArgumentException("In method swap of class "
            + "MaxHeap the input arrayIndex2 is not a valid node position");
}

It could be simply

if (arrayIndex1  this.numberOfNodes) {
    throw new ...
}
if (arrayIndex2  this.numberOfNodes) {
    throw new ...
}

-
Furthermore, you could extract out the checking logic (since it's duplicated, used twice for the two parameters and the same logic is in other methods too) to a validator method:

void swap(final int arrayIndex1, final int arrayIndex2) {
    checkValidIndex(arrayIndex1,
            "In method swap of class MaxHeap the input arrayIndex1 is not a valid node position: " + arrayIndex1);
    checkValidIndex(arrayIndex2,
            "In method swap of class MaxHeap the input arrayIndex2 is not a valid node position: " + arrayIndex2);
    ...
}

private void checkValidIndex(final int arrayIndex, final String message) {
    if (arrayIndex  this.numberOfNodes) {
        throw new IllegalArgumentException(message);
    }
}

Google Guava has similar checkArgument method which supports handy template strings (%s`) too. (See also: Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

-
I'd use a little bit shorter excepti

StackExchange Code Review Q#32269, answer score: 7