pereges said:
<snip>
> Can you please provide some general tips as to how I can improve my
> style of coding ?
I promised a reply to this, and here it is.
Your source, which I have made generally available at
http://www.cpax.org.uk/scratch/raytrace.zip
for the time being (a
"scratch" directory being exactly what it says on the tin), comprises 733
lines in .c files and 232 lines in .h files, a total of 955 lines. This is
split across 9 source files and 8 headers, a total of 17 sources. Mean
source size: around 56 lines per file. I mention these statistics because
someone else suggested elsethread that you've broken things up too much.
Personally, I disagree. If this is what you're comfortable with, then
that's absolutely fine.
Okay, so let's look at the code. You should of course read other people's
replies too, because I'm bound to miss something.
I indented the code to my own taste. It had tabs in, and they had to go,
because Usenet hates tabs. Line numbers refer to /my/ version of the code,
which was the result of indenting the original with these settings:
$ cat ~/.indent.pro
--blank-lines-after-declarations
--blank-lines-after-procedures
--blank-lines-before-block-comments
--braces-after-if-line
--brace-indent0
--braces-after-struct-decl-line
--blank-before-sizeof
--case-brace-indentation0
--continuation-indentation2
--case-indentation2
--break-function-decl-args
--honour-newlines
--indent-level2
--line-length72
--comment-line-length64
--continue-at-parentheses
--break-after-boolean-operator
--dont-cuddle-do-while
--dont-cuddle-else
--no-space-after-casts
--no-space-after-function-call-names
--no-space-after-parentheses
--dont-break-procedure-type
--no-space-after-for
--no-space-after-if
--no-space-after-while
--dont-space-special-semicolon
-nut
--preserve-mtime
--struct-brace-indentation0
(These settings were not kind to your
/* comments */
/* like */
/* this */
but that's minor. You can always re-run indent with your own settings when
you're ready.)
The first problem I found was that the project wouldn't compile. That
probably means that you are using C99isms - not wrong, but inconvenient
for those few remaining millions that don't have a C99 compiler. To get
your code to compile, I must either disable C90 conformance in my compiler
or hack the code. Since I don't plan on disabling conformance,
code-hackery happened at this end.
Rule 1 is of course to fix the first diagnostic message first. Now, in
this
case, the first few messages were:
raytrace.c: In function `find_closest_intersection':
raytrace.c:29: warning: `index' might be used uninitialized in this
function
raytrace.c: In function `raytrace':
raytrace.c:67: warning: unused variable `i'
The first of these looks dire, but it may well be that obj->ntri is always
greater than 0. At this stage, however, I don't know that. And it would
not hurt to initialise index to -1 at this stage. If it remains -1 after
the loop, the original program was clearly assuming that the loop was
always setting this value, and therefore was broken, so I'm going to
assert that it's not -1... in fact, I'm going to assert that it's
non-negative (because it's set to i, which runs from 0 upwards). This
involves adding <assert.h> to the include list, and changing
int index, i;
to
int index = -1, i;
and adding
assert(index >= 0);
just after the for-loop and just before res->t = closestdistance;
Second fix to this file: in the raytrace() function, i is unused, so I
removed it.
The next set of problems came in radar.c - and this was the showstopper
for
gcc, so let's see what we have to do to bring it into line. I don't expect
very much work will be required, since you seem to be able to compile it
okay yourself (so presumably this is just a matter of removing
extensions).
Here are the first few diagnostics:
radar.c: In function `gen_grid_points':
radar.c:28: warning: passing arg 1 of `sqrt' as floating rather than
integer due to prototype
radar.c:29: warning: passing arg 1 of `sqrt' as floating rather than
integer due to prototype
radar.c:32: parse error before `unsigned'
radar.c:41: parse error before `double'
Well, I had to dig through three layers of #include before finding an
include for math.h. Not funny. Still, it's there somewhere. The first two
warnings are a nuisance, but there's nothing actually wrong with the code
as such. I can certainly see why you're passing an integer value to a
function that takes double - and C /will/ provide a conversion, so that's
fine.
The other stuff? Well, not your fault - you're using valid C99 syntax -
but
my compiler doesn't sup****t it so I'm going to rewrite it into a syntax
that is sup****ted by your compiler and mine. I will take the op****tunity
to point out some assumptions that your code is making by asserting that
they are true. I've also re-written your malloc in canonical style:
*pointinarray = malloc(*numpoints * sizeof **pointinarray);
In general, for any object pointer p, you can point it at the memory for N
objects of that type by calling malloc like this:
p = malloc(N * sizeof *p);
Note the leading * on the rightmost p.
Plugging in: for N, you want *numpoints, right? So:
p = malloc(*numpoints * sizeof *p);
For p, you want *pointinarray. Watch carefully, counting stars:
*pointinarray = malloc(*numpoints * sizeof **pointinarray);
And that's it. This form survives a type change without modification
(except, of course, a type change to void *).
I am very suspicious about these lines:
for(yi = 0; yi <= numpointsy; ++yi)
{
for(xi = 0; xi <= numpointsx; ++xi)
How SURE are you that those <= are correct? It's very likely that they
should be < rather than <= but you as the code's author will be able to
work out whether this is an error far more quickly than I can.
My revision of your gen_grid_points function (which requires you to add an
include for the <assert.h> header) looks like this:
int gen_grid_points(vector ** pointinarray,
unsigned long int *numpoints,
double xmax,
double xmin,
double ymax,
double ymin,
double zdist)
{
unsigned long int numpointsx;
unsigned long int numpointsy;
unsigned long int i = 0;
double xlength = xmax - xmin;
double ylength = ymax - ymin;
double xsize;
double ysize;
unsigned long int xi, yi;
assert(pointinarray != NULL);
assert(numpoints != NULL);
assert(*numpoints >= 0);
assert(xmax >= xmin);
assert(ymax >= ymin);
numpointsx = sqrt(*numpoints);
numpointsy = sqrt(*numpoints);
assert(numpointsx > 1);
assert(numpointsy > 1);
xsize = xlength / (numpointsx - 1);
ysize = ylength / (numpointsy - 1);
*numpoints = (numpointsx + 1) * (numpointsy + 1);
*pointinarray = malloc(*numpoints * sizeof **pointinarray);
if(*pointinarray == NULL)
{
perror("malloc has failed");
return FAILURE;
}
/* Generating the points on the grid, points to be used as ray origin */
for(yi = 0; yi <= numpointsy; ++yi)
{
for(xi = 0; xi <= numpointsx; ++xi)
{
(*pointinarray + i)->x = xmin + xi * xsize;
(*pointinarray + i)->y = ymin + yi * ysize;
(*pointinarray + i)->z = zdist;
++i;
}
}
return SUCCESS;
}
I recommend that you plug this version in, and re-test. Do any of the
assertions fail? If so, that may well be your problem.
I then went on to look at initialize_radar(), but the scanf calls rather
put me off. Explaining how to use scanf correctly is terribly tedious. And
we're already up to 200+ lines in this reply. Bottom line, though: if you
must use it, at least check that the return value is correct.
This reply, though long, doesn't actually say very much. But what *does*
it
say? Well, maybe a summary is in order:
* I defended your decision to modularise early;
* I explained why I re-indented your code;
* I showed why it's a good idea to keep your code C90-conforming (it saves
time for those people who wish to help you but who don't have
C99-conforming compilers);
* I showed how to use assertions to verify that implicit program
assumptions are correct;
* I showed you the canonical, maintenance-friendly form of malloc call;
* I pointed out a couple of possible off-by-one errors;
* I recommended that you check the return value of scanf (if you must use
scanf at all).
It may well be that none of the above are causing your problem - but they
are all good suggestions anyway that fall into the category of "general
tips for improving coding" - as per your request.
If I get the time, I'll continue this review exercise later.
--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www.
+rjh@[EMAIL PROTECTED]
users: <http://www.cpax.org.uk/prg/writings/googly.php>
"Usenet is a strange place" - dmr 29 July 1999
|
