Organizing my code in C

Can some one please provide suggestions as to how I can improve the
organization of main file ?

In my ray tracing project, I have to carry out following task (in
sequence)

1. Read the mesh from an ascii file and store it in the mesh data
structure.
2. Create the kd tree for fast mesh traversal.
4. Create rays, trace them on the fly and calculate the scattered and incident
electric fields.

currently it looks like this:

Code:

---


#include "initialize.h"
#include "mesh.h"
#include "vector.h"
#include "kdtree.h"
#include "common.h"
#include "ray.h"
#include <stdio.h>
#include <stdlib.h>
 

/* read_mesh, init_plwave and create_kdtree are functions defined in initialize.c */
/* which basically open mesh file, allocate memory, scan some data from user etc */
/* and finally call the program which does the actual processing on the data structure */

int initialize(mesh **m, char *zfname, kdtree **tree)
{
 
  if(read_mesh(zfname, m) || init_plwave() || create_kdtree(tree, *m))
  {
    killall(); /* destroy all data structures */
    return (1);
  }
  return (0);
}

int main(void)
{
  mesh *m = NULL; /* pointer to mesh */
  kdtree *tree = NULL; /* pointer to kd tree */
  char *ptr = NULL;
  char filename[50];

  printf("Enter the file name\n");
  if(fgets(filename, sizeof(filename), stdin) == NULL)
  {
    error_report("Bad file name");
    exit(1);
  }

  if((ptr = strchr(filename, '\n')) != NULL)
    *ptr = '\0';
 
  if(initialize(&m, filename, &tree))
    exit(1);

 if(calc(m, tree))  /* do calculations */
 {
  killall();
  return (1);
 }

  print_result(); /* display result */ 

  killall(); /* destroy all data structures */

  exit(0);
}

---

Btw I'm planning to declare m, tree, filename as extern variables in common.h and define them in main.c because they are to persist in memory as long as the calculations are being done. Also, once they are initialized, they won't be changed during calculations. Is it really bad to use extern even if you know what you want to do.

>> I'm planning to declare m, tree as extern variables because they are to persist in
>> memory as long as the calculations are being done

That would be the wrong reason since extern has no bearing on the scope of a variable. extern tells the linker to look elsewhere for a variable because it is actually declared in another module.

If the source does what it's meant to there's not much to do here.

Why is exit() so appropriate for main() rather than a simple return statement?

Are you comfortable with how initialize_mesh() is written or could it be refactored? I'm not sure what those functions do but I would rather break it into several statements rather than chain them together with or:

Code:

---

int rv_read = read_mesh(zfname, m);
int rv_init = init_plwave();
int rv_tree = create_kdtree(tree, *m)
if(rv_read || rv_init || rv_tree )
  {
    killall(); /* destroy all data structures */
    return (1);
  }

---

It's simply cleaner because the functions would always be called top down and their return values could still be used to clean up. But whatever makes sense.

Quote:

---

Originally Posted by citizen

>> I'm planning to declare m, tree as extern variables because they are to persist in
>> memory as long as the calculations are being done

That would be the wrong reason since extern has no bearing on the scope of a variable. extern tells the linker to look elsewhere for a variable because it is actually declared in another module.

If the source does what it's meant to there's not much to do here.

Why is exit() so appropriate for main() rather than a simple return statement?

Are you comfortable with how initialize_mesh() is written or could it be refactored? I'm not sure what those functions do but I would rather break it into several statements rather than chain them together with or:

Code:

---

int rv_read = read_mesh(zfname, m);
int rv_init = init_plwave();
int rv_tree = create_kdtree(tree, *m)
if(rv_read || rv_init || rv_tree )
  {
    killall(); /* destroy all data structures */
    return (1);
  }

---

It's simply cleaner because the functions would always be called top down and their return values could still be used to clean up. But whatever makes sense.

---

what if its done *properly* ?
Well these data structures are frequently needed in many functions, and continously passing them everywhere is extermely painful even when they are hardly used in a function. It has made my code extermely obfuscated, difficult to read and probably even slower in cases where recursive functions are used. Let's say mesh *m is passed to a recursive function. Then in that case, for all the calls a seperate copy of m will be maintained even if m was utilized in probably 1 statement. Apart from this during the erro handling, it is quite easy to call the killall function from any where in the program where the error occured and free the data structures allocated upto that point. No need to pass any parameters.

eg :

Code:

---

void killall()
{
  if(m != NULL)
    killlmesh();
  if(tree != NULL)
    killtree();
}

---

The purists may moan, but if a data structure is used in almost every function, it seems natural to make it global, in the program. That's the purpose of global scope, imo.

Recursive calls with a non-global array. would only be passing a pointer, no matter how big the array was.

Nevertheless, an array that is used by nearly every function, should be carefully considered as a global array, at least.

I personally dislike returning true to indicate failure as it makes the progam logic seem wrong. I also second the point about simply using return statements from main. However it seems otherwise okay.

What about the error flag ? How about making it extern as well instead of propogating it every where and then back to main ?