0
\$\begingroup\$

After reading noobs, I'm interested in possible way to improve readability of the following short snippet.

  • a struct pictdb_file is a small file system containing a metadata array and a file descriptor fpdb.
  • the function allocates the resources, and forwards initialization to db_create_logic.

How can I improve the readability of this snippet ? Please comment about: the syntax, white-lines, comments, function extracting, etc.

Also, which version do you prefer and why ?

version 1:

enum error_codes
db_create_resources(const char* db_filename, struct pictdb_file* db_file)
{
    //allocate metadata
    db_file->metadata = calloc(db_file->header.max_files, sizeof(pict_metadata));

    if (db_file->metadata == NULL) {
        return ERR_OUT_OF_MEMORY;
    }

    //open fpdb
    db_file->fpdb = fopen(db_filename, "wb");

    if (db_file->fpdb == NULL) {
        free(db_file->metadata);
        db_file->metadata = NULL;
        return ERR_IO;
    }

    //forward
    enum error_codes e;
    e = db_create_logic(db_filename, db_file);

    //release resources
    free(db_file->metadata);
    db_file->metadata = NULL;
    db_close(db_file);

    return e;
}

version 2: (comment and blank-lines changes)

enum error_codes
db_create_resources(const char* db_filename, struct pictdb_file* db_file)
{
    db_file->metadata = calloc(db_file->header.max_files, sizeof(pict_metadata));
    if (db_file->metadata == NULL) {
        return ERR_OUT_OF_MEMORY;
    }

    db_file->fpdb = fopen(db_filename, "wb");
    if (db_file->fpdb == NULL) {
        free(db_file->metadata);
        db_file->metadata = NULL;
        return ERR_IO;
    }

    enum error_codes e;
    e = db_create_logic(db_filename, db_file);

    free(db_file->metadata);
    db_file->metadata = NULL;
    db_close(db_file);

    return e;
}
\$\endgroup\$
  • \$\begingroup\$ Not much to review, but also not much to change. It looks fine to me except I'd delete all of the comments. They're mostly useless because they say the same thing that the code does. \$\endgroup\$ – Edward May 24 '16 at 12:07
  • \$\begingroup\$ @Edward, exactly the kind of comment I hoped for, thanks. [I edited with an alternative] \$\endgroup\$ – Julien__ May 24 '16 at 12:16
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 May 24 '16 at 13:01
  • \$\begingroup\$ my mistake, sorry \$\endgroup\$ – Julien__ May 24 '16 at 13:06
4
\$\begingroup\$
  1. There's no specification. What does this function do? What arguments should I pass? What are the preconditions on db_file? (It looks as if I might have to fill in some of the fields, but which ones?)

  2. "resources" is spelled thus.

  3. The function does not check the preconditions.

  4. Mixing fopen and db_close seems confusing and error-prone. fopen should be paired with fclose, and db_close should be paired with db_open.

  5. If the call to fopen fails, it looks as if you lose information about the cause of the error. fopen can fail due to permissions (EACCES), lack of disk space (ENOSPC), missing directory (ENOENT), and so on, but these all get converted to ERR_IO.

  6. The function uses db_file->metadata to hold a pointer to some memory, but this memory is not actually used by the function, and by the end of the function the memory is freed and the pointer set to NULL. It seems wrong to use the db_file structure this way. If db_create_logic needs some temporary memory, it should allocate it itself (and preferably in a local variable, not in the db_file structure).

  7. The cleanup logic for db_file->metadata appears twice. This is a bad idea: if you changed it in one of the places, would you remember to change it in the other? Consider refactoring the error handling code to look like this:

    enum error_codes
    db_create_resources(const char* db_filename, struct pictdb_file* db_file)
    {
        enum error_codes e;
    
        db_file->metadata = calloc(db_file->header.max_files,
                                   sizeof *db_file->metadata);
        if (db_file->metadata == NULL) {
            e = ERR_OUT_OF_MEMORY;
            goto fail_calloc;
        }
    
        db_file->fpdb = fopen(db_filename, "wb");
        if (db_file->fpdb == NULL) {
            e = ERR_IO;
            goto fail_fopen;
        }
    
        e = db_create_logic(db_filename, db_file);
        db_close(db_file);
    fail_fopen:
        free(db_file->metadata);
        db_file->metadata = NULL;
    fail_calloc:
        return e;
    }
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer and interesting ideas. I edited my question to be more explicit about args checking and db_close(). \$\endgroup\$ – Julien__ May 24 '16 at 12:55
  • 1
    \$\begingroup\$ I rolled back your edit: it is confusing if the question no longer matches the answers. See the FAQ. \$\endgroup\$ – Gareth Rees May 24 '16 at 13:02
1
\$\begingroup\$

The version without comments looks fine to me. What I would say is that unless you're working in a very constrained system, or have significant memory requirements this:

db_file->fpdb = fopen(db_filename, "wb");

is far more likely to fail than this:

db_file->metadata = calloc(db_file->header.max_files, sizeof(pict_metadata));

With that in mind, I'd be inclined to do the memory allocation and file opening the other way around, however that's largely a matter of taste.

Does the db_close function call fclose on db_file->fpdb, if so this feels wrong, since you're explicitly calling fopen in your db_create_ressources method, I would expect the close in the same place.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks for you answer and remarks. I added an example about db_close()... but you're right this is confusing. db_close frees the memory and closes the file handler. So I guess that I could just call it instead of free() ... or not and be explicit even though that's code duplication ? \$\endgroup\$ – Julien__ May 24 '16 at 12:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.