code critique

Started by
16 comments, last by Fruny 18 years, 8 months ago
I wrote the following code to find the area of a triangle: inline float triarea(float x0, float y0, float x1, float y1, float x2, float y2) { #define U1 (x1 - x0) #define U2 (y1 - y0) #define V1 (x2 - x0) #define V2 (y2 - y0) #define CROSSPROD U1*V2 - U2*V1 #define AREA 0.5 * fabs(CROSSPROD); return AREA; } Later on, I realized that all the defines will be around later on. Any ideas on how to fix this while still making the code readable and fast? mike http://www.coolgroups.com/
Mike C.http://www.coolgroups.com/zoomer/http://www.coolgroups.com/ez/
Advertisement
#undef

OR just use variables.
"C lets you shoot yourself in the foot rather easily. C++ allows you to reuse the bullet!"
inline float triarea(float x0, float y0, float x1, float y1, float x2, float y2){     return 0.5 * fabs((x1 - x0) * (y2 - y0) - (y1 - y0) * (x2 - x0));}


?

All your #defines are redundant - there's no need for them.
If I undef and the macros are already defined, they will still be redefined and then undefined.

If I use variables it will be slower.

If I expand the macros, it will be less clear.

What I think I'd like is some sort of local macro or one that's limited to one file. Does such a beast exist?

mike
http://www.coolgroups.com/
Mike C.http://www.coolgroups.com/zoomer/http://www.coolgroups.com/ez/
Quote:Original post by mike74
#define U1 (x1 - x0)
#define U2 (y1 - y0)
#define V1 (x2 - x0)
#define V2 (y2 - y0)
#define CROSSPROD U1*V2 - U2*V1
#define AREA 0.5 * fabs(CROSSPROD);


NO. NO. *smack with rolled-up newspaper*

Quote:If I use variables it will be slower.

Really? Have you tried it, on a release build with full optimization?
Quote:If I expand the macros, it will be less clear.

If a person reading your code cannot understand or figure out this:
Quote:inline float triarea(float x0, float y0, float x1, float y1, float x2, float y2)
{
return 0.5 * fabs((x1 - x0) * (y2 - y0) - (y1 - y0) * (x2 - x0));
}

then they shouldn't be reading code in the first place.
inline float triarea(float x0, float y0, float x1,float y1, float x2, float y2){    const float u1 = x1 - x0;    const float u2 = y1 - y0;        const float v1 = x2 - x0;        const float v2 = y2 - y0;        const float crossprod = u1*v2 - u2*v1;    const float area = 0.5 * fabs( crossprod );    return area;      }


That's how I would write it. The const's are somehow overkill, but sometimes it helps catching bugs.
[Edit]Ok, I wouldn't write it that way, but it is better than using #define's.[/edit]
You're still creating all these unnecessary variables, which would impact performance.

mike
http://www.coolgroups.com/
Mike C.http://www.coolgroups.com/zoomer/http://www.coolgroups.com/ez/
Quote:Original post by mike74
You're still creating all these unnecessary variables, which would impact performance.

Again: have you tried this?
Quote:Original post by mike74
You're still creating all these unnecessary variables, which would impact performance.


No, I am creating CONSTANTS. Which to the compiler, are equivalent to the expressions with which I initialized them. They don't take up memory space during runtime.

This topic is closed to new replies.

Advertisement