Two variants of a picture database initialization routine

I wonder which of the two snippets is better? In the second one, I extracted two short subroutines.

• On the one hand, extraction clearly separate the two tasks

• On the other hand, it induces 6 more lines for function prototype

The code is about a simple images database called pictdb_file. The first function contributes to the creation of a new database.

/** this is a helper method for [...] blahblah */
static error_enum
db_create_delegate(const char* db_filename, pictdb_file* new_db_file)
{
    strncpy(new_db_file->header.db_name, CAT_TXT,  MAX_DB_NAME);
    new_db_file->header.db_name[MAX_DB_NAME] = '\0';

    new_db_file->header.num_files = 0;
    new_db_file->header.db_version = 0;

    size_t n1;
    n1 = fwrite(&new_db_file->header, sizeof(pictdb_header), 1, new_db_file->fpdb);
    if (n1 != 1) {
        return ERR_IO;
    }

    size_t n2;
    n2 = fwrite(new_db_file->metadata, sizeof(pict_metadata), new_db_file->header.max_files, new_db_file->fpdb);
    if (n2 != new_db_file->header.max_files) {
        return ERR_IO;
    }
    printf("%lu item(s) written\n", n1 + n2);
    return 0;
}

Compared to:

```
/** helper method [...] blahblah */
static error_enum
db_create_helper(const char db_filename, pictdb_file db_file)
{
finish_initialization(db_file);
return write_to_file(db_file);
}
static void
finish_initialization(pictdb_file* new_db_file)
{
strncpy(new_db_file->header.db_name, CAT_TXT, MAX_DB_NAME);
new_db_file->header.db_name[MAX_DB_NAME] = '\0';

new_db_file->header.num_files = 0;
new_db_file->header.db_version = 0;
}
static error_enum
write_to_file(pictdb_file* db_file)
{
size_t n1;
n1 = fwrite(&db_file->header, sizeof(pictdb_header), 1, db_file->fpdb);
if (n1 != 1) {
return ERR_IO;
}
size_t n2;
n2 = fwrite(db_file->metadata, sizeof(pict_metadata), db_file->header.max_files, db_file->fpdb);
if (n2 != db_file->header.max_files) {
return ERR_IO;
}
printf("%lu i

For me, there's two main things that drive extracting into a function.

• Is there scope for reusing the functionality?

• Would a function make the code easier to follow?

Looking at your code, it doesn't seem like the functions that you've implemented have much scope for reuse. Whilst they could make your code easier to follow, I don't think they do, primarily because the names don't really describe what they do. I find getting names right to be hard work, but well named methods make code a lot easier to follow.

Your method names that you've introduced are:

db_create_helper
    finish_initialization
    write_to_file

They don't tell me anything about what it is you're expecting the methods to do.

An alternative might be something like this:

initialize_db_file
    build_db_header
    write_db_header
    write_db_metadata

These names aren't perfect either, I don't really know what 'metadata' represents and if it should really be part of the header, which is why I've split your write_to_file in to two, however I've tried to name the methods so that they convey what it is I think the methods are doing. This makes the top level method more meaningful, describing the process of initializing the db file.

As far as method length goes, methods that are too long are a red flag, because they usually mean that too much is being done in one place and the method is likely to be hard to follow. However, I'm Ok with one line methods, as long as the name for the method actually conveys the intent better than the line it hides.

StackExchange Code Review Q#129229, answer score: 2