I recently got scolded real bad for writing the following:

/**
 * @brief Reads metadata from a .meta file associated with a <internal directory format> directory.
 *
 * This function reads the contents of the specified `.meta` file into the provided `buffer`,
 * reading up to META_MAX bytes and appending a null terminator to ensure it is a valid C string.
 * 
 * @note If fewer than META_MAX bytes are read, the result will still be null-terminated.
 *       The caller must ensure that `buffer` is at least META_MAX+1 bytes in size.
 *
 * @see <company custom dir type> specification at <internal link>
 * 
 * @param buffer    Pointer to a pre-allocated buffer of size at least META_MAX+1 where the metadata will be stored.
 * @param metaFile  File pointer to the opened .meta file.
 *
 * @return Returns 0 on success, or -1 if either parameter is NULL.
 */
int readMeta(char *buffer, FILE *metaFile)
{
    if (metaFile == NULL || buffer == NULL)
        return -1;

    buffer[fread(buffer, 1, META_MAX, metaFile)] = '\0';

    return 0;
}

I was told it's "stupid" to use the return value of fread() directly as an index in:

buffer[fread(buffer, 1, META_MAX, metaFile)] = '\0';

Notes:

1. The specification states that the meta file must contain META_MAX bytes or fewer, and it can be empty.

2. The parser that consumes this data is out of my control.

3. The function is only expected to be used with files known to meet the format spec.

Can someone explain why this is so wrong?

I recently got scolded real bad for writing the following:

/**
 * @brief Reads metadata from a .meta file associated with a <internal directory format> directory.
 *
 * This function reads the contents of the specified `.meta` file into the provided `buffer`,
 * reading up to META_MAX bytes and appending a null terminator to ensure it is a valid C string.
 * 
 * @note If fewer than META_MAX bytes are read, the result will still be null-terminated.
 *       The caller must ensure that `buffer` is at least META_MAX+1 bytes in size.
 *
 * @see <company custom dir type> specification at <internal link>
 * 
 * @param buffer    Pointer to a pre-allocated buffer of size at least META_MAX+1 where the metadata will be stored.
 * @param metaFile  File pointer to the opened .meta file.
 *
 * @return Returns 0 on success, or -1 if either parameter is NULL.
 */
int readMeta(char *buffer, FILE *metaFile)
{
    if (metaFile == NULL || buffer == NULL)
        return -1;

    buffer[fread(buffer, 1, META_MAX, metaFile)] = '\0';

    return 0;
}

I was told it's "stupid" to use the return value of fread() directly as an index in:

buffer[fread(buffer, 1, META_MAX, metaFile)] = '\0';

Notes:

1. The specification states that the meta file must contain META_MAX bytes or fewer, and it can be empty.

2. The parser that consumes this data is out of my control.

3. The function is only expected to be used with files known to meet the format spec.

Can someone explain why this is so wrong?

• c

• 4 @Mio, seeing the larger context would help rate the quality of this approach. – chux Commented 22 hours ago
• 3 Propbably this person was thinking that fread returns negative number on error :). – 0___________ Commented 22 hours ago
• 2 It returns the number of objects read , not the number bytes read - unless the size of the object is 1 (which in this case it is - assuming buffer is a char array). – 001 Commented 22 hours ago
• 2 @001 how does it matter in this case? – 0___________ Commented 22 hours ago
• 2 It is bad style, hard to read, you do not know how many elements you read - so you will need to traverse the buffer to know. Much easier to have an additional variable. – 0___________ Commented 22 hours ago

1 Answer 1

Can someone explain why this is so wrong?

Too fully address this, we need to know more about the overall situation.
Yet from what is already presented, some problems are evident.

Loss of information

To recover the length read, code might use strlen(buff), yet that is wrong when the data read includes a null character and is inefficient when the immediate following code needs the length. Better to save the return value of fread().

Why fread() for reading strings?

OP's talked about reading a string (see edit history) and then is attempting to form a C string.

Commonly when a variable amount of characters is read into a C string, one is trying to read a line and fgets() is the better approach.

Note that fread() will not stop reading if it reads a '\n' or '\0'.

Perhaps OP's wants to read the entire file then it makes some sense? Unfortunately OP has not yet posted much more of the larger context.

Weak goal "goal is to read the first n-1 chars (or less if there are fewer than n-1 chars)"

The biggest issue with this is lacking distinction between reading n-1 characters and detecting there are more.

size_t amount_read = fread(buff, sizeof(buff[0]), n /* -1 */, file);
if (amount_read == n) {
  ; // TBD code to handle more;
} else {
  buff[amount_read] = '\0';
}

Other weaknesses

Check for failure
All I/O functions, especially reads, deserve a check to see if anything was read. OP's still does not want to post the larger context so I'll assume the code does not vector on nothing read. Consider instead:

size_t amount_read = fread(buff, sizeof(buff[0]), n-1, file);
if (amount_read == 0) {
  break;
}
buff[amount_read] = '\0';

Type sign-ness change
fread(buff, sizeof(buff[0]), n-1, file) has an int n-1 where a size_t (an unsigned type) is the argument type. This implies OP is not compiling with a warning to flag such issues. For a learner, better to enable lots of warnings and avoid casual sign-ness changes. I would consider fread(buff, sizeof buff[0], sizeof buff / sizeof buff[0] - 1, file).

Origin of n
Knowing the origin of n and insuring it is more than zero and not too large is important. Maye it was a constant, maybe not?

"n" is an int
Pedantic concern: certainly a problem when n <= 0 or (rarely) n > SIZE_MAX. For array sizing and indexing, look to using size_t.

Unneeded ()
A minor WET/DRY style issue

// buff[fread(buff, sizeof(buff[0]), n-1, file)] = '\0';
buff[fread(buff, sizeof buff[0], n-1, file)] = '\0';