FIFO queue for a 2D array in ANSI C, version 2

Question

This is a modification of my previous version.

I would like advice on improving a FIFO algorithm for a 2D array that I've developed in ANSI C.

I have written code that uses this algorithm for test purposes. Random numbers are put into an array until all elements are full. The program then asks the user what numbers need to go in to the 2D array in order to update it. The code then removes the oldest array, shifts all remaining arrays up by one, and then adds the new array (as input by the user).

I am asking for a review of the FIFO code, fifo_algorithm; the surrounding code is only for testing purposes. I would appreciate any suggestions that only use standard C.

code:

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

void random_num_input(void);
void print_random_num_input(void);
void print_new_array(void);
void fifo_algorithm(void);

int user_num[5] = { 0 };
uint16_t big_array[10][5] = { 0 };
uint16_t big_array_copy[10][5] = { 0 };
uint16_t array_print[1] = { 0 };
uint8_t count = 0;
int main()
{
    random_num_input();
    printf("These are the original numbers in the big array:\n");
    print_random_num_input();
    while (count < 10)
    {
        printf("Please enter values to swap in:\n");
        for (int i = 0; i < 5; i++)
        {
            scanf("%d", &user_num[i]);
        }
           printf("\n\n");
           fifo_algorithm();
        count++;
    }
}
//stores random numbers between 0-255 into big_array and big_array_copy
void random_num_input(void)
{
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array[j][i] = rand() % 255;
        }
    }
    
}
//prints the random values within the big_array 
void print_random_num_input(void)
{
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            array_print[0] = big_array[j][i];
            printf("%d,", array_print[0]);
        }
        printf("\n");
    }
}
//This algorithm removes the oldest array in big_array_copy (element 9), shifts every
//element up by 1 and stores the user inputted array into element 0. Thus a fifo that updates
//the 2d array with the user input values every time 
void fifo_algorithm(void)
{
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array_copy[j][i] = big_array[j][i];
        }
    }
    for (int k = 0; k < 5; k++)
    {
        big_array_copy[10 - 1][k] = 0;
    }
    for (int j = 9; j > 0; j--)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array_copy[j][i] = big_array[j - 1][i];
        }
    }
    for (int i = 0; i < 5; i++)
    {
        big_array_copy[0][i] = user_num[i];
    }
    print_new_array();
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array[j][i] = big_array_copy[j][i];
        }
    }
}
//simply prints the new, updated values in big_array_copy
void print_new_array(void)
{
    printf("These array values have been swapped:\n ");
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            array_print[0] = big_array_copy[j][i];
            printf("%d,", array_print[0]);
        }
        printf("\n");
    }
}

Thanks

Answer 1

Good formatting

Good use of (u)intN_t types

When printing, use a matching format specifier: printf("%" PRI16u , array_print[0]);

Avoid naked magic numbers

Define constants for flexibility and clarity

// int user_num[5] = { 0] };
//uint16_t big_array[10][5] = { 0 };

#define BIG_ARRAY_MAXJ 10
#define BIG_ARRAY_MAXI 5
int user_num[BIG_ARRAY_MAXI] = { 0 };
uint16_t big_array[BIG_ARRAY_MAXJ][BIG_ARRAY_MAXI] = { 0 };

Instead of 9, use BIG_ARRAY_MAXJ - 1, etc.

To address the "I could have put macros down to name 5 and 10 but I didn't see how that could be advantageous," comment: using named constants convey to others, including yourself at a later time, the relationship amongst the many constants and their individual meaning.

Global objects

Typically, it is far better to code for use of local objects than global ones.

Instead of

random_num_input(void)

Pass in a pointer to the array and its dimensions. If VLA is supported:

random_num_input(int jn, int in, uint16_t a[j][i]) {
  for (int j = 0; j < jn; j++) {
    for (int i = 0; i < in; i++) {
        a[j][i] = rand() % 256;
    }
}

Array indexing

For big arrays, consider size_t instead of int to well handle large arrays. size_t is the Goldilocks size for array sizing and indexing: not too small, not too big.

For big arrays, likely also need to move to allocated memory rather than fixed sizes.

Comment/code off by 1.

rand() % 255 forms values [0...254], not [0...255].

// stores random numbers between 0-255 into big_array ...
        big_array[j][i] = rand() % 255;

Consider

big_array[j][i] = rand() % 256;
// or 
big_array[j][i] = (uint8_t) rand();

Copying arrays

With arrays of the same dimension and type, as here, could use memcpy().

// for (int j = 0; j < 10; j++) //
//    for (int i = 0; i < 5; i++) {
//        big_array_copy[j][i] = big_array[j][i];
//    }
//}

// if known to be non-overlapping 
memcpy(big_array_copy, big_array, sizeof big_array_copy); 
// else 
memmove(big_array_copy, big_array, sizeof big_array_copy);

Minor: Tidier print

Consider not printing a trailing ','. Little reason for a global array_print[]. Use the correct portable specifier

    char *sep = "";
    for (int i = 0; i < BIG_ARRAY_MAXI; i++) {
        printf("%s%" PRI16u , sep, big_array_copy[j][i]);
        sep = ",";
    }
    printf("\n");

Answer 2

Passing everything in global variables might seem convenient for this small program, but is a poor practice more generally, since it makes it hard to reason about any function in isolation. In particular, big_array_copy, array_print and count seem to be used only for short-term storage, so should be local to their respective functions.

The functions are tightly bound to those globals, and to the particular array sizes you have chosen. If you ever want to change any of the dimensions, you'll need to re-understand the entire program to make the many changes that are required. You could save all that effort by simply writing a couple of manifest constants for the dimensions, and using them consistently. That also helps readers, who no longer need to keep referring back to the definitions to check whether the bounds are correct (and remember, your code will be read many more times than written!).

It's not clear why user_num holds int values, as they get silently converted to uint16_t:

    big_array_copy[0][i] = user_num[i];

This problem could be avoided by declaring user_num to be an array of uint16_t.

There's no need for big_array_copy - just move the items within big_array and then copy the new items into it:

void fifo_algorithm(uint16_t const *user_num)
{
    memmove(big_array+1, big_array, sizeof big_array - sizeof big_array[0]);
    memcpy(big_array[0], user_num, sizeof big_array[0]);
}

Not only is that much shorter than copying one element at a time to big_array_copy and back, it's also much less work for CPU and cache, too.

The input handling is very weak - if we mistakenly provide a non-numeric input, the scanf() will repeatedly fail trying to parse it.

It's good to see you've included a test program. You could improve on that by making the tests self-checking and providing their own input. As it is, the test requires a separate input file (or even worse, manual entry of data) and it requires visual inspection of the output, both of which make it unsuitable for automated regression testing.

---

Improved code

#include <inttypes.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static void random_num_input(void);
static int read_user_values(uint16_t *user_num);
static void print_array(void);
static void fifo_algorithm(uint16_t const *user_num);

/* Global FIFO object */
#define COLS 5
#define ROWS 10
uint16_t big_array[ROWS][COLS] = { 0 };

int main(void)
{
    random_num_input();
    puts("These are the original numbers in the big array:");
    print_array();
    for (int count = 0;  count < ROWS;  ++count) {
        uint16_t user_num[COLS] = { 0 };
        int status = read_user_values(user_num);
        if (status) {
            return status;
        }
        printf("\n\n");
        fifo_algorithm(user_num);
        print_array();
    }
}

// Populate the FIFO with random numbers between 0-255
void random_num_input(void)
{
    for (int j = 0;  j < ROWS;  ++j) {
        for (int i = 0;  i < COLS;  ++i) {
            big_array[j][i] = (uint16_t)(rand() & 0xff);
        }
    }
}


static int read_user_values(uint16_t *user_num)
{
    puts("Please enter values to swap in:");
    for (int i = 0;  i < COLS;  ++i) {
        int status = scanf("%" SCNu16, &user_num[i]);
        if (status == EOF) {
            fputs("Failed to read input\n", stderr);
            return EXIT_FAILURE;
        } else if (status != 1) {
            /* discard and re-read this datum */
            fprintf(stderr, "Failed to parse item %d; Re-enter last %d value(s)\n",
                    i, COLS-i);
            scanf("%*[^\n]");
            --i;            /* repeat this scanf */
        }
    }
    return 0;                   /* success */
}


// Prints the contents of the FIFO
void print_array(void)
{
    for (int j = 0;  j < ROWS;  ++j) {
        for (int i = 0;  i < COLS;  ++i) {
            printf("%" PRIu16",", big_array[j][i]);
        }
        printf("\n");
    }
}

// Shift a new row of data into the FIFO
void fifo_algorithm(uint16_t const *user_num)
{
    memmove(big_array+1, big_array, sizeof big_array - sizeof big_array[0]);
    memcpy(big_array[0], user_num, sizeof big_array[0]);
}