HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Heapsort is not modular

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
heapsortmodularnot

Problem

For the time being, I am not interested in any recursive solution. The code is not modularized; the whole body is inside the main function.

#include
#include
/*This code is written by NF between any time of 120711 and 210711;modified at 231211*/

int main()
{
    int length=0,min_heapified=0,start_length=0,index=0;
    cout>length;
    int array[length];
    for(int i=0;i0);

            int swap=array[0];
            array[0]=array[--length];/*modification done at this point to avoid keeping the sorted elements into another array;and swapping done between the 0th and last element*/
            array[length]=swap;
            cout0);

  return 0;

}

Solution

int length=0,min_heapified=0,start_length=0,index=0;


I prefer one declaration per line because it's easier to read and find the type of variables. Furthermore, min_heapified is only used inside the do-while loop, so declare it there:

do {       
    int min_heapified = 0;
    do {
...


Change

if(x<=(length-1))


to

if (x < length)


It's more common and simple.

Instead of

int swap=array[left];
array[left]=array[root];
array[root]=swap;


you should create a swap function. Maybe it already exists in a library.

int swap(array, index, rootIndex) {
    int swap = array[index];
    array[index] = array[rootIndex];
    array[root] = swap;
}


If I'm right you could extract the following to a new function:

if(array[left]<array[root])
{
    int swap=array[left];
    array[left]=array[root];
    array[root]=swap;
    min_heapified=1;    
}


It looks for me that the other if do the same. The only difference is the used index, so I'd write something like this:

int swapIfLess(array, index, rootIndex) {
    if (array[index] < array[rootIndex]) {
        swap(array, index, rootIndex)
        return 1;   
    }
    return 0;
}


if (left < length) {
    if (swapIfLess(array, left, root)) {
        min_heapified=1;    
    }
}
if (right < length) {
    if (swapIfLess(array, right, root)) {
        min_heapified=1;    
    }
}


The i variable looks unnecessary:

int root=i;


Use the root variable in the for loop:

for (int root = 0; root < length; root++)


Maybe you should rename it to rootIndex.

Extract more functions. The for loop (inside the do-while loops) looks as a good candidate for this. Additionally, you should have a readInputArray function and a printResults function too.

Code Snippets

int length=0,min_heapified=0,start_length=0,index=0;
do {       
    int min_heapified = 0;
    do {
...
if(x<=(length-1))
if (x < length)
int swap=array[left];
array[left]=array[root];
array[root]=swap;

Context

StackExchange Code Review Q#6196, answer score: 7

Revisions (0)

No revisions yet.