Finding database matches and storing them in a glycopeptide structure

Question

I am relatively new to C and would like some feedback on a function that I have written, if it adheres to C standards or if there are some other things which I could have done better/differently.

/* Function to find DB matches and store them in a glycopeptide structure */
/* Written by Bas Jansen, 18-01-2012, Leiden University Medical Center */
glycopeptide*
databasereader (float precursor, float massaccuracy, MYSQL *connection) {
   float prec_lower = precursor - massaccuracy;         /* define lower border by substracting mass accuracy from given precursor */
   float prec_higher = precursor + massaccuracy;        /* define upper border by adding mass accuracy to given precursor */
   glycopeptide* glycan;                                /* define a pointer to the structure glycopeptide as glycan */
   char querystring[MAX_SIZE_LARGE];                    /* define a variable to contain the SQL query */
   sprintf(querystring,"select precursor.mzValue as 'M/z Value', glycoPeptide.description as 'Description', spectrum.spectrum as 'Data Type', binaryDataArray.arrayData as 'Data (encoded)', binaryDataArray.arrayLength as 'Array length', binaryDataArray.compLength as 'Compressed length' from glycoPeptide, spectrum, binaryDataArray, run, precursor where run.glycoPeptide = glycoPeptide.id AND spectrum.run = run.id AND precursor.run = run.id AND binaryDataArray.spectrum = spectrum.id AND precursor.mzValue between %f and %f ORDER BY glycoPeptide.description, spectrum.spectrum", prec_lower, prec_higher);                    /* convert the border values into strings and put them in the SQL query */
   if (mysql_query (connection, querystring) != 0) {    /* checks if query gives any results */
       printf("Made it to the query part, but no hits\n");      /* give a message if the query gave no hits */
   } else {                                             /* if query gives hits do... */
       glycan = malloc(sizeof(glycopeptide)*1000);      /* allocate memory for the glycan structure */
       int teller = 0;                                  /* implements a counter to count and walk through the daughters of glycan */
       MYSQL_RES *result_set;                           /* defines a result_set structure */
       result_set = mysql_store_result (connection);    /* stores result from sql query in result_set structure */
       if (result_set == NULL) {                        /* checks if result from the query got stored */
           printf("mysql_store_result() failed");       /* print error if no result was stored */
       } else {                                         /* if query was successful... */
           MYSQL_ROW row;                               /* defines a row structure */
           int i;                                       /* defines a int to use in a for loop */
           while ((row = mysql_fetch_row (result_set)) != NULL) {       /* as long as there are rows in result set do... */
               for (i = 0; i < mysql_num_fields (result_set); i++) {    /* for each field in result row */
                   if (i == 0) {                                        /* if field is 'M/z Value' do... */
                       float buffer = atof(row[i]);                     /* convert string in to the float buffer variable */
                       (glycan+teller)->prec = buffer;                  /* assign the float buffer variable to the structure */
                   }                                                    /* closes the if field is (M/z Value) do... */
                   if (i == 1) {                                        /* if field is 'Description' do... */
                       (glycan+teller)->desc = malloc(sizeof(char)*MAX_SIZE_SMALL);     /* allocate memory for the description */
                       (glycan+teller)->desc = row[i];                  /* assign the string in field[i] to the structure */
                   }                                                    /* closes the if field is 'Description' do... */
                   if (i == 2) {                                        /* if field is 'Data Type' do... */
                       (glycan+teller)->datatype = malloc(sizeof(char)*MAX_SIZE_TINY);  /* allocate memory for the datatype */
                       (glycan+teller)->datatype = row[i];              /* assign the string in field[i] to the structure */
                   }                                                    /* closes the if field is 'Data Type' do... */
                   if (i == 3) {                                        /* if field is 'Data (encoded)' do... */
                       (glycan+teller)->binary = malloc(sizeof(char)*MAX_SIZE_LARGE);   /* allocate memory for the binary data */
                       (glycan+teller)->binary = row[i];                /* assign the string in field[i] to the structure */
                   }                                                    /* closes the if field is 'Data (encoded) do... */
                   if (i == 4) {                                        /* if field is 'Array length' do... */
                       int buffer = atoi(row[i]);                       /* convert string in to the int buffer variable */
                       (glycan+teller)->array_length = buffer;          /* assign the int buffer variable to the structure */
                   }                                                    /* closes the if field is 'Array length' do... */
                   if (i == 5) {                                        /* if field is 'Compressed length' do... */
                       int buffer = atoi(row[i]);                       /* convert string in to the int buffer variable */
                       (glycan+teller)->compressed_length = buffer;     /* assign the int buffer variable to the structure */
                   }                                                    /* closes the if field is 'Compressed length' do... */
               } /* closes for loop */
           teller++;                                    /* increments counter that counts and walks through the daughters of glycan */
           } /* closes while loop */
       /* Freeing memory */
       return(glycan);                                  /* returns the structure back to main */
       for (i = 0; i < teller; i++) {                   /* for loop that walks through all daughters of glycan */
           free((glycan+i)->desc);                      /* free the memory allocated to description of this particular daughter */
           free((glycan+i)->datatype);                  /* free the memory allocated to datatype of this particular daughter */
           free((glycan+i)->binary);                    /* free the memory allocated to binary of this particular daughter */
       }
       for (i = teller; i > 0; i--) {                   /* for loop that walks (backwards) through all daughters of glycan */
           (glycan+i)->desc=NULL;                       /* sets the pointer to description to NULL of this particular daughter */
           (glycan+i)->datatype=NULL;                   /* sets the pointer to datatype to NULL of this particular daughter */
           (glycan+i)->binary=NULL;                     /* sets the pointer to binary to NULL of this particular daughter */
   }
       free(glycan);                                    /* free the memory allocated for glycan */
       glycan = NULL;                                   /* sets the pointer to glycan to NULL */
       } /* closes the else, belonging to result_set */
   } /* closes the else, belonging to mysql_query */
} /* closes the function */

Here are my structs and #defines:

#define MAX_SIZE_TINY 128
#define MAX_SIZE_SMALL 512
#define MAX_SIZE_LARGE 1024

typedef struct {
    float prec;
    char* desc;
    char* datatype;
    char* binary;
    int array_length;
    int compressed_length;
} glycopeptide;

Answer 1

I'm not too familiar with C, so just some notes.

1. Comments like these are redundant, so they are unnecessary:

   MYSQL_ROW row;  /* defines a row structure */
   free(glycan);   /* free the memory allocated for glycan */
   

2. Statements after return look unreachable:

   return (glycan);
   for (i = 0; i < teller; i++) {
   

   So, this loop never runs.

   On the other hand freeing resources at the end of the function looks unnecessary, since the function returns with glycan and clients probably need this data.

3. Instead of the else branches like

   if (mysql_query (connection, querystring) != 0) {
     printf("Made it to the query part, but no hits\n");
   } else {                                             
     glycan = malloc(sizeof(glycopeptide)*1000);
     ...
   

   I'd return immediately:

   if (mysql_query (connection, querystring) != 0) {
     printf("Made it to the query part, but no hits\n");
     return; // a return value maybe neccesary
   }
   glycan = malloc(sizeof(glycopeptide) * 1000);
   

   It helps flatten the code.

Answer 2

Comments:

• Terrible comments (can not express that enough)

• In C one of the hard points is cordinating ownership of dynamically allocated memory.

  • You should have a big comment at the top of the function explaining that the returned pointer is dynamically allocated with detailed instructions on how to make sure it is de-allocated.
  • I would avoid asking the user to call free() as this binds your interface to providing memory allocated from malloc/calloc/realloc and this may be stiffling in future versions. A better alternative is to provide a free function that when passed the result of this function correctly tides up the pointer (and all sub pointers).

• If the function is expected to return a value you must return a value.

  • It looks like there are several ways to fall out of your code without returning anything. This is undefined behavior, you must return something even if it is NULL.
• Your error messages are not accurate:
  • printf("Made it to the query part, but no hits\n");
    Here your query failed. This does not mean there are not hits it means there was some form of failure in the query.
• Useless statement: glycan = NULL; (glycan goes out of scope the next statement).
• This works (glycan+teller)->STUFF but is a little hard to read.
  • I find the form glycan[teller].STUFF is a little more intuitive to read.

• It helps when you line up the code consistently:

         } /* closes for loop */
     teller++;                                    /* increments counter that counts and walks through the daughters of glycan */
     } /* closes while loop */
  

  • The Line teller++ should be indented another 4 character.
    This is making the code hard to read.
• Your code assumes that number of rows read from the DB is exactly dividable by 6.
  • If this does not hold then you free statements are freeing pointers that have not been allocated (and have not previously set to NULL).

• In this section:

     for (i = 0; i < teller; i++) {                   /* for loop that walks through all daughters of glycan */
         free((glycan+i)->desc);                      /* free the memory allocated to description of this particular daughter */
         free((glycan+i)->datatype);                  /* free the memory allocated to datatype of this particular daughter */
         free((glycan+i)->binary);                    /* free the memory allocated to binary of this particular daughter */
     }
     for (i = teller; i > 0; i--) {                   /* for loop that walks (backwards) through all daughters of glycan */
         (glycan+i)->desc=NULL;                       /* sets the pointer to description to NULL of this particular daughter */
         (glycan+i)->datatype=NULL;                   /* sets the pointer to datatype to NULL of this particular daughter */
         (glycan+i)->binary=NULL;                     /* sets the pointer to binary to NULL of this particular daughter */
      }
  

  • There is no point in doing two loops.
    You should have initialized all the pointers to NULL on allocation. (calloc may help)
    Then in the above code you should be setting only the used pointers back to NULL (as the others are already NULL). Here you have the worst of all worlds, you are resetting the unused ones and freeing but not resetting the used ones. Its a good job this code is unreachable.