Finding database matches and storing them in a glycopeptide structure

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 g

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 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.

StackExchange Code Review Q#7912, answer score: 4