Printing a heartbeat (heart star) animation

Question

Here is a C program to print a heart star animation using a while loop written by myself.

I've tested it on Ubuntu 18.04 (Bionic Beaver) and Ubuntu 20.04 (Focal Fossa).

To animate it, I print a sequence of heart star patterns in different sizes.

#include <stdio.h>
#include <unistd.h>

int main() {
    int i, j, h, k = 0;

    while (k < 99) {
        /* code */
        h = k % 3;
        printf("\e[1;1H\e[2J");
        printf("\e[?25l");

        i = 0;
        while (i < 3) {
            j = 0;
            while (j < (2 - i) * 2 + h) {
                printf(" ");
                j++;
            }

            j = 0;
            while (j < (i + 2) * 2 - h * 2) {
                printf("* ");
                j++;
            }

            j = 0;
            while (j < (2 - i) * 2 + 1 + h) {
                printf("  ");
                j++;
            }

            j = 0;
            while (j < (i + 2) * 2 - h * 2) {
                printf("* ");
                j++;
            }

            printf("\n");
            i++;
        }
        i = 0;
        while (i < 9) {
            j = 0;
            while (j < i * 2 + h * 2) {
                printf(" ");
                j++;
            }
            j = 0;
            while (j < 17 - i * 2 - h * 4) {
                printf("* ");
                j++;
            }
            printf("\n");
            i++;
        }

        fflush(stdout);
        usleep(200000);
        k++;
    }
}

Answer 1

Welcome to CodeReview! The program doesn't look too bad, however there are some ways to improve it:

Naming things

The biggest issue with your code is that there is a bunch of single-letter variable names (i...k), some magic constants (2, 3, 4, 17 and 99), and nested while-loops. If I wasn't told what this program would do and didn't run it for myself, I would have no clue reading this code what it is going to do. However, if you would give (better) names to things, it might explain what everything means to the reader. That includes you yourself in the future!

Let's start with the outer loop:

int main() {
    for (int i = 0; i < 99; i++) {
         int size = i % 3;
         clear_screen();
         draw_heart(size);
         usleep(200000);
    }
}

Keeping i as a generic loop counter is OK, as this is so commonplace that a longer name is not going to make this more readable. But I renamed h to size. Then I assume you have created two functions, one for clearing the screen, and another for drawing a heart of a given size. If you now read this code, you can very quickly see that you are repeatedly drawing a heart of a varying size, without requiring any comments.

Here is the function to clear the screen:

void clear_screen(void) {
    // Use ANSI escape codes to clear the screen
    printf("\e[1;1H\e[2J");
    printf("\e[?25l");
}

The name of the function alone is already enough to know what the function tries to do, the comment here is to explain the weird characters that are printed. Someone who doesn't recognize these escape codes now knows what these are, and can search for "ANSI escape codes" to find out more about them.

The constants you have in your code could be given names, like so:

void draw_heart(int size) {
    static const int top_size = 3;
    static const int bottom_size = 9;
    ...
}

And then used consistently in the calculations you use, such that if you want to change the size of the heart, you can just change these constants.

Use for-loops where appropriate

You are using a lot of while-loops in cases where you can more clearly write for-loops. Any time you initialize a variable, run while the variable satisfies some condition, and then update that variable, use for, like so:

void draw_heart(int size) {
    // Draw two small upward pointing triangles
    for (int i = 0; i < 3; i++) {
        for (int j = 0; i < (2 - i) * 2 + size; j++) {
           printf(" ");
        }

        ...
    }

    // Draw one large downward pointing triangle
    for (int i = 0; i < 9; i++) {
        ...
    }
}

The advantage is that in one line you combine all this information, so just from reading that line you can see what the start condition is, what the end condition is and how the variable is updated every step.

Answer 2

If you are going to write raw terminal codes to output, I'd advise encapsulating that into a well-named function. Few of us can tell by looking exactly what effect \e[1;1H\e[2J\e[?25l will have.

There's a lot of use of while loops that, while not functionally incorrect, would be more idiomatically written as for loops, enabling other developers to read and understand the code more quickly. E.g.

            j = 0;
            while (j < (2 - i) * 2 + h) {
                printf(" ");
                j++;
            }

can be rewritten as

            for (int j = 0;  j < (2 - i) * 2 + h;  j++) {
                printf(" ");
            }

It might be worth giving a name to the calculation (2 - i) * 2 + h here.

If we know the upper bound for that value, we could avoid the loop, by printing a substring of a larger string:

char many_spaces[90];  /* my guess at largest bound */
memset(many_spaces, ' ', sizeof many_spaces);
...
printf("%.*s", (2 - i) * 2 + h, many_spaces);

A similar trick could be used for the other loops that repeat the same string many times.

Answer 3

Aside from anything else, like how you might want to save your output as a string and print that, the usleep() function is deprecated and has been removed from version 4 of the Single Unix Specification.

Either use nanosleep(), or #define _XOPEN_SOURCE and _POSIX_C_SOURCE to values that will select a version of the system libraries that still supports usleep(). Otherwise, your source might not compile. I’d recommend changing to nanosleep() and also specifying the feature-test macros for it.

So:

#define _XOPEN_SOURCE   700
#define _POSIX_C_SOURCE 200809L

#include <stdio.h>
#include <unistd.h>

/* ... */

  static const struct timespec to_sleep_200ms = {
    .tv_sec = 0,
    .tv_nsec = 200000000
  };
  
  nanosleep( &to_sleep_200ms, NULL );

In theory, defining _XOPEN_SOURCE is enough, but defining both is good defensive coding in case someone later defines only the other. For example, if you had specified only _POSIX_C_SOURCE for an older version and someone later compiled with -D_XOPEN_SOURCE=700, that would remove the declaration of usleep() and your program would no longer compile. So, specify both. That way, if anyone ever merges in a version clash, you’ll get a compilation error immediately that tells you what the bug is.

In a project with more than one source file, you probably want to move the feature-test macros into a new header file or the makefile.