current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

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 ressources, 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_ressources(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 ressources
    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_ressources(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;
}
share|improve this question
    
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. – Edward May 24 at 12:07
    
@Edward, exactly the kind of comment I hoped for, thanks. [I edited with an alternative] – Julien__ May 24 at 12:16
1  
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. – Vogel612 May 24 at 13:01
    
my mistake, sorry – Julien__ May 24 at 13:06
up vote 4 down vote accepted

  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;
}
share|improve this answer
    
Thank you for your answer and interesting ideas. I edited my question to be more explicit about args checking and db_close(). – Julien__ May 24 at 12:55
1  
I rolled back your edit: it is confusing if the question no longer matches the answers. See the FAQ. – Gareth Rees May 24 at 13:02
    
I accepted your answer as it was very useful. thanks. – Julien__ May 24 at 15:51
up vote 1 down vote

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.

share|improve this answer
    
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 ? – Julien__ May 24 at 12:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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