Downheap on array giving wrong output

Question

I am writing a program to execute a heap sort. When I try to execute the removeMin function, and a downheap, it seems that I am always getting incorrect output.

For example if I input 10 integers in this order:

3, 6, 8, 3, 89, 35, 7, 9, 1, 4

I expect

1, 3, 3, 4, 6, 7, 8, 9, 35, 89

But I get:

1, 3, 3, 4, 6, 7, 8, 9, 35, 35

Here is my code heap code:

public class heapsort {

private Integer[] myHeap;
private int size;
private int maxSize;

public heapsort (int x){
    myHeap = new Integer[x+1];
    maxSize=x;
    size=0;
}

public void min(){
    if (isEmpty())
        System.out.print("Is empty");
    else
        System.out.println(myHeap[0]);
}

public boolean isEmpty(){
    return (size==0);
}

public int size(){
    return size;
}

public void insert(int x){
    if (size==maxSize)
        System.out.println("Heap is full");
    else{
        size++;
        myHeap[size] = x;
        upheap(size);
    }
}

public void upheap(int x){
    int temp;
    if (x>1){
        if (myHeap[x]<myHeap[x/2]){
            temp = myHeap[x];
            myHeap[x]=myHeap[x/2];
            myHeap[x/2]=temp;
            upheap(x/2);
        }
    }   
}

public void removeMin(){
    int temp;
    temp = myHeap[1];
    myHeap[1]=myHeap[size-1];
    size--;
    if (size>1){
        downheap(1);
    }
    System.out.println(temp);
}

public void downheap(int x){
    int temp;

    int temp, minIndex, left=2*x, right=2*x+1;

    if (right>=size){
        if (left>=size)
            return;
        else
            minIndex=left;
    }

    else{
        if (myHeap[left] <= myHeap[right])
            minIndex = left;
        else
            minIndex = right;
    }

    if (myHeap[x] > myHeap[minIndex]){
        temp = myHeap[minIndex];
        myHeap[minIndex]=myHeap[x];
        myHeap[x]=temp;
        downheap(minIndex);
    }
}

Followed by my main program:

public static void main (String[] args){
    int i=0;
    Scanner input = new Scanner(System.in);
    System.out.println("Enter array size: ");
    int n = input.nextInt();

    heapsort myArray = new heapsort(n);
        System.out.println("Please input integers");
    for (i=1; i<=n; i++){
        myArray.insert(input.nextInt());
    }

    while (!myArray.isEmpty()){
        myArray.removeMin();
    }
}
}

Just a thought, in `removeMin()`, should that be `if (size >= 1)`? — Ole V.V., Mar 01 '17 at 06:14
Did you try a debugger? Another trick, find the minimal example that gives the error. Can you reproduce the error with just three integers? Two integers? One integer? — Ole V.V., Mar 01 '17 at 06:17
I'm pretty sure that the line `myHeap[1]=myHeap[size-1];` in `removeMin()` should be `myHeap[1]=myHeap[size]`. If you insert one item, when `insert()` finishes, `size` is set to 1. If you then called `removeMin()`, it would return `myHeap[0]`, which is some undefined value. — Jim Mischel, Mar 02 '17 at 21:24
In general, your code suffers from using item 1 as the root. Change your code so that the root is at `myHeap[0]`. The index calculations are: `left=2*i+1` and `right=2*i+2`, and a node's parent is `(i-1)/2`. By making everything 0-based, you won't confuse yourself so much. — Jim Mischel, Mar 02 '17 at 21:26

Ole V.V. · Answer 1 · 2017-03-04T12:04:55.483

It seems to me from a few runs that your sort drops one element underway and then prints the last element twice. If I enter an array size of two and numbers 3 and 4 (or 4 and 3), I get

3
3

I think this is a simple enough case that you should be able to think about what happens in the removeMin method. Your array is [null, 3, 4] (because you are never using the 0th element) and size is 2. What will temp be? Which array element are you copying into myHeap[1]? What is the new size? Will downheap(1) be called or not? Remember that indices are 0-based. I am deliberately leaving the answers to you, I trust you will figure out and be able to fix your bug.

PS I have fixed what I thought was the bug. Now if I enter an array size of 3 and numbers 1 2 3, I get

1
3
2

So either I was wrong or there’s one more bug in there. Hope you will be able to figure out. It’s good to train your debugging skills, it won’t be the last time you will need them.

PPS Your min method is wrong, it doesn’t matter since you are not using it. And you are declaring temp twice in downheap() (you probably have discovered already that it doesn’t compile).

Edit: warning: spoiler

Stephen O’C, I believe you have found your bugs by now, so I don’t do any harm to your learning by confirming what you know, and someone else reading along might be interested in knowing too. So here are the bugs I found and how to fix them.

The first error is in the removeMin() method. This line

    myHeap[1] = myHeap[size - 1];

should be

    myHeap[1] = myHeap[size];

We want to move the last element of the heap to the top. Since elements are in indices 1 throgh size, we should not subtract 1 from size (had we used the array as 0-based, size - 1 would have been correct. This error caused whatever element to end last in the heap to be missed — often one of the greater elements.

With this fix, the code could appear to work correctly, but now and then swapped two elements in the sorted output. The bug is in these two lines of downheap():

    if (right >= size) {
        if (left >= size)
            return;

If right or left, respectively, is equal to size, they still point to an element of the heap, so in this case we need to do the sifting operation and cannot return at this point. It’s an error very similar to the first one. The fix is to use strict greater than operators:

    if (right > size) {
        if (left > size)

With this fix I have not been able to spot any more bugs.

Downheap on array giving wrong output

1 Answers1