Formatted output of a structure

Question

I've implemented a to_string function that takes a player_t instance and format it into a string.

Code:

#include <stdio.h>
#include <string.h>

typedef struct player {
  int id;
  float points;
  char display_name[256];
} player_t;

#define MAX_OUTPUT_SIZE sizeof(int) + sizeof(float) + 256

void to_string(player_t, char [MAX_OUTPUT_SIZE]);

int main()
{
  char stringified[MAX_OUTPUT_SIZE];

  player_t p1 = {.id = 1, .points = 17.4, .display_name = "Tester"};

  to_string(p1, stringified);

  printf("player: %s\n", stringified);

  return 0;
}

void to_string(player_t player, char output[MAX_OUTPUT_SIZE])
{
  char temp[MAX_OUTPUT_SIZE];

  snprintf(temp, sizeof(temp), "\n\tid: %d", player.id);
  strncat(output, temp, 6 + 1);

  snprintf(temp, sizeof(temp), "\n\tpoints: %f", player.points);
  strncat(output, temp, 10 + 9);

  snprintf(temp, sizeof(temp), "\n\tdisplay_name: %s", player.display_name);
  strncat(output, temp, 18 + 256);
}

Output:

player:
    id: 1
    points: 17.400000
    display_name: Tester

Questions:

1. Is there a better way to define MAX_OUTPUT_SIZE instead of manually adding the size of each element?
2. Is there a better way to implement to_string instead of using a temporary variable, the separate combination of strncat & snprintf and explicitly counting the characters for strncat?.
3. Am I over complicating my usage of snprintf & strncat?
4. (slightly out of topic) Do you recommend any C project to read it's code that implement similar functionalities and is following best practices?

Thanks in advance and I'm sorry if I've missed any obvious mistakes (still in the process of learning C with Ruby/JS background).

Answer 1

The MAX_OUTPUT_SIZE constant does not correctly account for how many characters might be generated, and it does not agree with the calculation made within to_string(). In particular, note that sizeof (int) and sizeof (float) indicate how much memory is required to store values of those types; that's not at all related to how many characters are required to print them.

The correct calculation is hard to get right: you need to add the longest int value (formatted as a decimal), the longest float value (as a float) and the longest display_name, as well as all your fixed text and the terminating NUL.

It's hard to do that portably.

It should also be noted that a function parameter of char[MAX_OUTPUT_SIZE] decays to a pointer, and you have no guarantee that it points to an array of the declared size.

The right way to output into a char array is to pass the length as a separate parameter, and for your output function to indicate the required length if it fails (either by the return value, or via an output parameter). Consider a function interface similar to snprintf() - but be aware of the common ways of mis-calling snprintf() and make sure you avoid them.

Other comments:

• Avoid passing your player by value - it's much cheaper to pass a pointer to a readonly struct.
• With the code below, we can check the worst-case output by constructing a struct player with id of INT_MIN, points of -FLT_MAX and a maximum-length display_name, and a debug version will tell us how big a buffer is required. You can then modify the constant to the value indicated.
• If you later internationalize the program, the fixed portion of the strings are no longer fixed. You might even have to resort to the technique of calling snprintf(NULL, 0, format, args...) to measure the size, then allocating sufficient memory with malloc(). That adds to your work, as you then need to ensure that the allocated memory is correctly freed.

#include <stdio.h>
#include <string.h>

typedef struct player {
  int id;
  float points;
  char display_name[256];
} player_t;

/* Will write no more than length characters, including the terminating NUL.
   The return value indicates the number of characters to be written, excluding the NUL.
   A return value of length or more indicates a truncated write */
size_t to_string(char *output, size_t length, const player_t *player)
{
    size_t required_len =
        snprintf(output, length,
                 "\n\tid: %d\n\tpoints: %f\n\tdisplay_name: %s",
                 player->id, player->points, player->display_name);
#ifndef NDEBUG
    if (required_len >= length)
        fprintf(stderr,
                "Buffer too small: needs %zd but %zd was supplied\n",
                required_len+1, length);
#endif
    return required_len;
}

static const size_t MAX_OUTPUT_SIZE = 5;

#include <limits.h>
#include <float.h>
int main()
{
  char stringified[MAX_OUTPUT_SIZE];

  player_t p1 = {.id = 1, .points = 17.4, .display_name = "Tester"};

  if (to_string(stringified, sizeof stringified, &p1) >= sizeof stringified) {
      fprintf(stderr, "Buffer too small!\n");
      return 1;
  }

  printf("player: %s\n", stringified);
  return 0;
}