-1

I have a reverse sorted heap. I am trying to build a max heap:

Code I have:

    int main(int argc, char *argv[])
{
int heapArray[] = {0, 1, 2, 3, 4, 5, 6 , 7, 8 ,9 ,10 , 11, 12, 13 ,14 ,15};
int n = sizeof(heapArray)/sizeof(int);

printTree(heapArray, n);
buildHeap(heapArray, n);
printTree(heapArray, n);
}

void buildHeap(int array[], int n)
{
printf("buildHeap\n");
int i = (n-1)/2;
while(i > 0) heapify(array, n, i--);
}

void heapify(int array[], int n,  int i)
{
printf("heapify [%i] = %i\n", i, array[i]);
int childLeft = 0, childRight = 0;
int largest = i;
// printf("largest init: %i\n", largest);
if(array[2*i]) childLeft = 2*i; 
if(array[2*i + 1]) childRight = 2*i + 1;
printf("child left [%i] = %i  child right [%i] = %i\n", childLeft, array[childLeft], childRight, array[childRight]);
if(array[childLeft] > array[i]) largest = childLeft;
// printf("largest after cL compare: %i\n", array[largest]);
if(array[childRight] > array[largest]) largest = childRight;
// printf("largest after cR compare: %i\n", array[largest]);
if(largest != i)
{
    swap(array, i,largest);
    heapify(array, n, i);
}


}

void swap(int array[], int indexA, int indexB)
{
printf("swap [%i] %i with [%i] %i\n", indexA, array[indexA], indexB, array[indexB]);
int temp = array[indexA];
array[indexA] = array[indexB];
array[indexB] = temp;
}

Currently the recursive call to heapify is not fully sorting the heap. Do I need to do call maxheap multiple times? It seems the only way to get the max heap

oCodaa
  • 173
  • 2
  • 3
  • 13

1 Answers1

5

Very subtle problem! Apart from various small annoyances which meant your code would not run until I fixed a few things (function prototypes and #includes, and the absence of a printTree() function), your real issue is in the line

while(i > 0) heapify(array, n, i--);

This never calls heapify with i==0, since you use the post-increment operator (i-- instead of --i. Thus, the last call to heapify has i==1.

So the first change you need is this:

i = n / 2;
while(i > 0) heapify(array, n, --i);

There is also a problem with your heapify code. First of all, when you look for children, you test whether the array element is zero; and if it is, you set the child node to node 0 (which is actually a valid node). I changed things a bit so you start out by initializing the left and right child to -1, so you can tell the difference between "valid" and "invalid".

Finally - when you make a swap, you need to recurse back down the tree; and you were looking at the wrong leg ( you were re-examining the one that just got swapped, instead of drilling down):

swap(array, i,largest);
heapify(array, n, i);

Instead of

swap(array, i,largest);
heapify(array, n, largest);

Taking it all together, the code becomes as follows:

#include <stdio.h>

void buildHeap(int array[], int n);
void heapify(int array[], int n,  int i);
void swap(int array[], int indexA, int indexB);
void printTree(void);

int* TREE; // global variable used for printing the entire tree

int main(int argc, char *argv[])
{
int heapArray[] = {0, 1, 2, 3, 4, 5, 6 , 7, 8 ,9 ,10 , 11, 12, 13 ,14 ,15};
int n = sizeof(heapArray)/sizeof(int);
TREE = heapArray;

printTree();
buildHeap(heapArray, n);
printTree();
return 0;
}

void printTree(void)
{
// lazy way to print the entire tree...
  int* array = TREE;
  printf("                        %3d\n ", array[0]);
  printf("            %3d                     %3d\n", array[1], array[2]);
  printf("      %3d         %3d         %3d         %3d\n", array[3], array[4], array[5], array[6]);
  printf("   %3d   %3d   %3d   %3d   %3d   %3d   %3d   %3d\n", array[7], array[8], array[9], array[10], array[11], array[12], array[13], array[14]);
  printf("%3d\n", array[15]);

  printf("\n");
}

void buildHeap(int array[], int n)
{
  printf("buildHeap\n");
  // changed starting condition
  int i = n/2;
  // changed from i-- to --i so we get to zero
  while(i > 0) heapify(array, n,--i);
}

void heapify(int array[], int n,  int i)
{
  printf("heapify [%i] = %i\n", i, array[i]);
  printTree();
  // mark nodes initially as -1 to distinguish from "valid" zero
  int childLeft = -1, childRight = -1;
  int largest = i;

  // changed the way we check for valid nodes:
  if(2*i+1<n) childLeft = 2*i+1;
  if(2*i + 2<n) childRight = 2*i + 2;

  // see if any nodes are invalid now:
  if(childLeft < 0 && childRight < 0) return;
  if(childLeft < 0) childLeft = childRight;
  if(childRight < 0) childRight = childLeft;

  printf("child left [%i] = %i  child right [%i] = %i\n", childLeft, array[childLeft], childRight, array[childRight]);
  if(array[childLeft] > array[i]) largest = childLeft;
  if(array[childRight] > array[largest]) largest = childRight;
  if(largest != i)
  {
    swap(array, i,largest);
    heapify(array, n, largest);
  }
}

void swap(int array[], int indexA, int indexB)
{
  printf("swap [%i] %i with [%i] %i\n", indexA, array[indexA], indexB, array[indexB]);
  int temp = array[indexA];
  array[indexA] = array[indexB];
  array[indexB] = temp;
}

And the sorted tree at the end of this is

                         15
              10                      14
        8           9          12          13
     7     1     0     4    11     5     2     6
  3

The head of each node is now bigger than the children below it - just as you would expect from this algorithm.

Floris
  • 45,857
  • 6
  • 70
  • 122
  • just small note that if we have the size of the array, then we should avoid using recursion by using loop instead. – Dzung Nguyen Nov 18 '14 at 04:19
  • @nXqd - can you give a reason for your statement ("should avoid")? I am not a CS person and am always looking to learn. How would this work with a loop? – Floris Nov 18 '14 at 11:49
  • You can read the bottom section of this paper about non recursively heapify. The idea of avoiding recursion is because the recursive function will be put into stack, and the stack is limited. So in case we have a pretty complicated and long array to heapify, we got the error : Stack Overflow. http://homepages.math.uic.edu/~leon/cs-mcs401-s08/handouts/nearly_complete.pdf – Dzung Nguyen Nov 19 '14 at 10:47
  • 1
    Interesting handout - thanks for that link. I believe that in the worst possible case, your recursion would go log(n) levels deep... If your n elements fit in memory it's hard to see that this would lead to stack overflow. Is that right? At any rate, it's good to see that there is an alternative. – Floris Nov 19 '14 at 14:17
  • 1
    The complexity of the algo would be logn in the worst case but still, the limitation of stack is around http://stackoverflow.com/questions/2656722/is-there-a-limit-of-stack-size-of-a-process-in-linux . The space complexity would be much bigger, and of course you can tweak the limitation of stack :D Just to make sure, it works in normal system. – Dzung Nguyen Nov 20 '14 at 07:53
  • I agree with nXqd, apart from memory, performance is also an issue. So, if neat recursive solution is there avoid recursion. Here space shouldn't be a problem though as stack will take log(n) with a very less arg overhead – Pervez Alam Mar 21 '17 at 12:05