2
\$\begingroup\$

I've written a program showing the process of bubble sort arithmetic in C. I need some suggestions on improving it.

#include <stdio.h>
#include <Windows.h>
#include <stdlib.h>
#include <time.h>
#define SIZE 20
#define WIDE (SIZE * 2)

void RepaintScreen(int *pData, int count, int p1, int p2)
{
    system("cls");
    char b[(WIDE + 1) * (SIZE)];
    b[0] = '\0';
    for (int i = 0; i < count; i++)
    {
        if (i == p1 || i == p2)
        {
            strcat(b, "-->");
        } else {
            strcat(b, "   ");
        }
        for (int j = 0; j < pData[i]; j++)
        {
            strcat(b, "|");
        }
        strcat(b, "\n");
    }
    printf("%s", b);
}

void BubbleSort(int *pData, int count)
{
    int iTemp;
    for (int i = 0; i < count; i++)
    {
        for (int j = count - 1; j > i; j--)
        {
            if (pData[j] < pData[j - 1])
            {
                iTemp = pData[j];
                pData[j] = pData[j - 1];
                pData[j - 1] = iTemp;
                RepaintScreen(pData, count, i, j); //refresh the screen
                Sleep(10);
            }
        }
    }
}

int main( int argc, char **argv )
{
    int data[SIZE];
    srand(time(NULL));
    //initiate data
    for (int i = 0; i < SIZE; i++)
    {
        data[i] = rand() % WIDE;
    }
    BubbleSort(data, SIZE);
    return 1;
}
\$\endgroup\$
6
  • \$\begingroup\$ I would create the line to print first and than print with one printf-call. Or even create all lines to print first and than make only one printf-call for all lines together. \$\endgroup\$
    – MrSmith42
    Commented Oct 28, 2013 at 10:57
  • \$\begingroup\$ Thank you! I rewrite the RepaintScreen function with printf call for all lines, and it works better. But it flickers sometimes, it may be the problem of system call "cls". \$\endgroup\$
    – Raphael
    Commented Oct 29, 2013 at 2:31
  • 1
    \$\begingroup\$ I'm sorry I clicked the reopen link too fast, saw your recent edit and assumed you had fixed your code; please edit the title of this question so it doesn't look like a StackOverflow question, remove references to the flickering problem and you might get more reopen votes :) \$\endgroup\$ Commented Oct 29, 2013 at 3:03
  • \$\begingroup\$ @retailcoder: Got it! \$\endgroup\$
    – Jamal
    Commented Oct 29, 2013 at 4:35
  • \$\begingroup\$ @retailcoder: In fact, the filckering problem is still existing, so I said it may be the problem of system call "cls". \$\endgroup\$
    – Raphael
    Commented Oct 29, 2013 at 15:28

1 Answer 1

1
\$\begingroup\$

Your code works nicely.

In BubbleSort I would move the definition of iTemp to the point of first use. The name iTemp is ugly and the 'i' makes me think you are using a type prefix (as does the 'p' in pData). I don't know anyone who thinks that is a good idea - just use temp and data.

In RepaintScreen I don't see the need to buffer the data yourself. You can just use stdio to buffer it for you:

static void RepaintScreen(const int *data, int count, int p1, int p2)
{
    for (int i = 0; i < count; i++) {
        fputs((i == p1 || i == p2) ? "-->" : "   ", stdout);

        for (int j = 0; j < data[i]; j++) {
            putc('|', stdout);
        }
        putc('\n', stdout);
    }
}

If you do accumulate a string, using strcat is inefficient and prone to buffer overflow. It is inefficient because on each call it must traverse the whole string to find the end. It would be better to keep track of the end of the string yourself. For example:

static void RepaintScreen(const int *data, int count, int p1, int p2)
{
    system("cls");
    char buf[(WIDE + 1) * (SIZE)];
    char *b = buf;

    for (int i = 0; i < count; i++) {
        if (i == p1 || i == p2) {
            *b++ = '>';
        } else {
            *b++ = ' ';
        }
        for (int j = 0; j < data[i]; j++) {
            *b++ = '|';
        }
        *b++ = '\n';
    }
    *b++ = '\0';
    fputs(buf, stdout);
}

Overflow occurs if you get the size of the buffer size wrong. Avoiding overflow is messy as you must check that there is enough space each time you insert into the buffer and exit sensibly if there is not. Using fputs/putc, as shown above, avoids overruns.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.