Export dataset to CSV in C

Question

I've just learnt the basics of C and I've built a working program that reads the dataset provided by my lecturer and exports it out to a CSV.

However, he said that this could've been simplified (a lot!) to just print out the data values in the required columns (x, y, z). But I don't see how that could work given my set up.

This is my first time coding (I'm a health student) and am a little overwhelmed. As I said, I'd just like my code to be reviewed and ways to make it simpler and just 'print out' the values.

Void create_CSV and int main are created by myself, whereas gen_sally is created by my lecturer.

#include<stdio.h>
#include<math.h>
#include<string.h>
#include<stdlib.h>
void gen_sally( int xs, int ys, int zs, int time, float *sally )
/*
 * Gen_Sally creates a vector field of dimension [xs,ys,zs,3] from
 * a proceedural function. By passing in different time arguements,
 * a slightly different and rotating field is created.
 *
 * The magnitude of the vector field is highest at some funnel shape
 * and values range from 0.0 to around 0.4 (I think).
 *
 * I just wrote these comments, 8 years after I wrote the function.
 * 
 * Developed by Sally of Sally University
 *
 */
{
 float x, y, z;
 int ix, iy, iz;
 float r, xc, yc, scale, temp, z0;
 float r2 = 8;
 float SMALL = 0.00000000001;
 float xdelta = 1.0 / (xs-1.0);
 float ydelta = 1.0 / (ys-1.0);
 float zdelta = 1.0 / (zs-1.0);
 for( iz = 0; iz < zs; iz++ )
 {
 z = iz * zdelta; // map z to 0->1
 xc = 0.5 + 0.1*sin(0.04*time+10.0*z); // For each z-slice, determine the spiral circle.
 yc = 0.5 + 0.1*cos(0.03*time+3.0*z); // (xc,yc) determine the center of the circle.
 r = 0.1 + 0.4 * z*z + 0.1 * z * sin(8.0*z); // The radius also changes at each z-slice.
 r2 = 0.2 + 0.1*z; // r is the center radius, r2 is for damping
 for( iy = 0; iy < ys; iy++ )
 {
 y = iy * ydelta;
 for( ix = 0; ix < xs; ix++ )
 {
 x = ix * xdelta;
 temp = sqrt( (y-yc)*(y-yc) + (x-xc)*(x-xc) );
 scale = fabs( r - temp );
/*
 * I do not like this next line. It produces a discontinuity 
 * in the magnitude. Fix it later.
 *
 */
 if ( scale > r2 )
 scale = 0.8 - scale;
 else
 scale = 1.0;
 z0 = 0.1 * (0.1 - temp*z );
 if ( z0 < 0.0 ) z0 = 0.0;
 temp = sqrt( temp*temp + z0*z0 );
 scale = (r + r2 - temp) * scale / (temp + SMALL);
 scale = scale / (1+z);
 *sally++ = scale * (y-yc) + 0.1*(x-xc);
 *sally++ = scale * -(x-xc) + 0.1*(y-yc);
 *sally++ = scale * z0;
 }
 }
 }
}
void create_csv(char* filename,float *sally, int size){
 printf("1");
 printf("\n Creating %s.csv file",filename);
 FILE *fp;
 fp=fopen(filename,"w");
 fprintf(fp,"X,Y,Z\n");
 int i;
 int counter = 0;
 for(i = 0; i< size; i++){
 if(sally[i] == 0){
 fprintf(fp,"0");
 } else {
 fprintf(fp,"%f",sally[i]);
 }
 counter++;
 if(counter == 3){
 fprintf(fp, "\n");
 counter = 0;
 } else {
 fprintf(fp,",");
 }
 }
 fclose(fp);
 printf("\n %sfile created",filename);
}
int main(int argc, char *argv[]){
 printf("1\n");
 //read from args
 int xs;
 int ys;
 int zs;
 int time;
 sscanf(argv[1],"%d",&xs);
 sscanf(argv[2],"%d",&ys);
 sscanf(argv[3],"%d",&zs);
 sscanf(argv[4],"%d",&time); // Is a constant, will always be reads as 1
 int arraySize = xs*ys*zs*1;
 //allocate memeory for array. This is done so that stack memory doesn't run out.'
 float* sally;
 sally = (float*)malloc((arraySize) * sizeof(float));
 //runs the code. One of the args is a pointer so no return type is needed. 
 gen_sally(xs,ys,zs,time,sally);
 //create varibles for file generation
 char filename[20] = "results.csv";
 create_csv(filename, sally, arraySize);
 free(sally);
 return 0;
}

• yuri: You probably mean create_csv as there is no create_sally function.
• JimmyNeedles: Thank you. Still doesn't help with my query though.

Answer 1

In main() there is the line

 int arraySize = xs*ys*zs*1;

Anything times 1 is itself so there is no reason to multiply by 1.

In the create_csv() function, the variable counter is not needed - the variable i can be used instead by using the modulo operator:

 if (i % 3) {
 fprintf(fp, ",");
 }
 else {
 fprintf(fp, "\n");
 }

An alternate simplification of this could be:

 fprintf(fp, "%c", (i % 3)? ',' : '\n');

Also in create_csv(), you could get by with a single printf() statement rather than two:

 printf("1\n Creating %s.csv file\n", filename);

In main(), a better programming practice for the malloc() would be to use

sizeof *sally

rather than

sizeof(float)

because the type of sally may change at some point; by using *sally only one line of code needs to change rather than multiple lines of code. The cast of the result from malloc() is not necessary.

 float* sally;
 sally = malloc((sizeof *sally) * arraySize);

This code is questionable:

 if (sally[i] == 0) {
 fprintf(fp, "0");
 }
 else {
 fprintf(fp, "%f", sally[i]);
 }

because it is comparing integer zero with a float variable, it might be safer to use 0.0 in the comparison. The current code may not work properly on all computers.

Was it a requirement to write a zero value as 0? If not, the code can be further simplified to

void create_csv(char* filename, float *sally, int size) {
 printf("1\n Creating %s.csv file\n", filename);
 FILE *fp;
 fp = fopen(filename, "w");
 fprintf(fp, "X,Y,Z\n");
 int i;
 for (i = 0; i < size; i++) {
 fprintf(fp, "%f%c", sally[i], (i % 3) ? ',' : '\n');
 }
 fclose(fp);
 printf("\n %sfile created", filename);
}

• Toby Speight: I've made some minor edits, and I also changed around the multiplying by sizeof - it's good practice to put the sizeof first, which matters when the number of elements itself is a product (because multiply associates to the left, and we want intermediates to be at least size_t).
• chux: Disagree with "current code may not work properly on all computers" A compliant compiler will convert the 0 to 0.0f and then do the comparison.

Answer 2

Always check whether I/O operations were successful

Code like this is problematic:

FILE *fp = fopen(filename, "w");
fprintf(fp,"X,Y,Z\n");

If we failed to open the file we'll get a NULL pointer assigned to fp. If we're really unlucky, the program will survive passing that to fprintf(), and we'll have no indication that the data were not saved. That can be a serious issue if it means that the user believes their data have been safely stored.

Similarly, we should check the return value of sscanf() when used on the arguments, before we attempt to use the values we read. (Alternative - consider strtoul() or strtoul(), so we can validate that there's no junk following a valid number).

Answer 3

float v double

Variables are float yet constants (0.4, 0.1,...) and functions (sin(), cos()) are double. It would make more consistent to use float or double throughout.

 float xc, time, z;
 xc = 0.5f + 0.1f*sinf(0.04f*time + 10.0f*z); 
 // or 
 double xc, time, z;
 xc = 0.5 + 0.1*sin(0.04*time + 10.0*z); 

accuracy v speed

hypot() is usual more accurate, although sometime slower than posted code.

temp = sqrt( (y-yc)*(y-yc) + (x-xc)*(x-xc) );
// Alternative: 
temp = hypot(y-yc, x-xc);
temp = hypotf(y-yc, x-xc); // using float only.

%f v %g

Recall that floating point numbers have a floating point. With "%f", large numbers will print with many unnecessary characters and small numbers will print 0.000000. It is more informative (especially during debug) to print the leading digits than to a fixed decimal point format. Suggest using "%g" or "%e".

// fprintf(fp,"%f",sally[i]);
fprintf(fp,"%g",sally[i]);

Answer 4

Inconsistent/lack of spacing

Please, don't do it like this: 0.1 + 0.4 * z*z + 0.1 * z * sin(8.0*z);, surround all the mathematical operators with spaces everywhere. Not just in random places.

Comments

Please, don't add wall of comments between function definition and it's body. It's really hard to read. Usually the function is commented BEFORE the function definition, so... following would be prefered:

/*
* Gen_Sally creates a vector field of dimension [xs,ys,zs,3] from
* a proceedural function. By passing in different time arguements,
* a slightly different and rotating field is created.
* @param[in] 
* ...
* @param[out]
*/
void gen_sally(int xs, int ys, int zs, int time, float *sally)

Following comments also are quite messy to read:

 z = iz * zdelta; // map z to 0->1
 xc = 0.5 + 0.1*sin(0.04*time+10.0*z); // For each z-slice, determine the spiral circle.
 yc = 0.5 + 0.1*cos(0.03*time+3.0*z); // (xc,yc) determine the center of the circle.
 r = 0.1 + 0.4 * z*z + 0.1 * z * sin(8.0*z); // The radius also changes at each z-slice.
 r2 = 0.2 + 0.1*z; // r is the center radius, r2 is for damping

Line breaks, etc. Why not just picking better names for variables instead? And describing everything what's gonna happen before actual code?

Sanity check

Your not checking for NULL pointer, invalid values, etc. None of Your functions can fail and return error, etc

• JimmyNeedles: Thanks for the input, but as i said in the question, the part of the code that you're commenting on is of my lecturers. Where as mine is below, that's the code i want to improve.
• Kamiccolo: sorry, missed that part :)