Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > Paranoid about style/elegance and function size, please quell

Reply
Thread Tools

Paranoid about style/elegance and function size, please quell

 
 
wired
Guest
Posts: n/a
 
      06-28-2003
Hi,

I've just taught myself C++, so I haven't learnt much about style or
the like from any single source, and I'm quite styleless as a result.
But at the same time, I really want nice code and I go to great
lengths to restructure my code just to look concise and make it more
manageable.
When I say this, I'm also referring to the way I write my functions.
It seems to me sometimes that I shouldn't have many void functions
accepting addresses and changing their values for example, but rather
a double function that returns the calculated value. i.e.
void Change(Type& variable);
should be replaced with
Type Change(Type variable);

I just don't know what's better code (in terms of speed, style, etc.).

I'm moving into a more mature phase of my program, and before I
continue I would like jsut a final verdict on my code. I'm looking for
comments about whether my functions should be more simple (i.e. too
much happens in each), whether I should rather return value instead of
alter through references passed, whether my general style is
inelegant. I'm also concerned about my class structures. But I'll show
you _some_ code and let you decide.

Please note that I'm not asking you to go through the actual logic of
all this code, but rather just calls to functions, classes and the
like.

Thanks.

Davin.

//---------------------
//Globals.cpp - just some global variables needed by most functions
#include <vector>
#include "GLFiles.h"
#include "Derived.h"

using std::vector;

//quadratic to draw cylinders, spheres etc.
GLUquadricObj *quadratic;

//window dimensions
int WIDTH = 1024;
int HEIGHT = 768;

//table dimensions
float length = 25.15f; // 251.5cm
float width = 13.84f;
float height = 8.13f;
float goal = 4.0f;

float rest = 1.5f; // width between esdge and tabletop

// Misc

int step = 150; // number of steps the collision detection will
deconstruct each translation into in checking for a collision

Mallet* Mallet1;
Mallet* Mallet2;
vector <Puck*> Pucks;

double friction = 0.985;
int mouse_sens = 4; // 4-10, 10-least sensitive

//---------------------
//Abstract.h - two abstract classes, some derived classes will be
inherited from both
#ifndef Abstract_Classes
#define Abstract_Classes

#include "Globals.h"

using namespace std;

class Movable;
class Drawable;

void AddDraw(Drawable* draw);
void AddMove(Movable* move);

extern double friction;

class Drawable
{

public:

Drawable() {
AddDraw(this);
}

virtual void Draw() = 0;

};

class Movable
{

public:

Movable(double x1, double y1, double r, Controller cont) {

AddMove(this);

control = cont;

x = x1;
y = y1;

x_change = 0.0;
y_change = 0.0;

radius = r;

}

double next_x() { return x + friction * x_change; }
double next_y() { return y + friction * y_change; }

double change_x() { return x_change; }
double change_y() { return y_change; }

double now_x() { return x; }
double now_y() { return y; }

double get_radius() { return radius; }
double get_mass() { return mass; }

Controller controller() { return control; }

friend void Collide();
friend void HitTable(Movable* obj, double change);

void Replace(); // reset its position (after a goal for example) and
velocity

virtual void Move() = 0;

protected:

Controller control;

double radius;
double mass; //used as a comparison between inherited types (as well
as the physics)

double x;
double y;

double x_change;
double y_change;

};

#endif

//------------------
//Derived.h - two derived classes
#ifndef Derived_Classes
#define Derived_Classes

#include "Abstract.h"

class Mallet: public Drawable, public Movable
{

public:

Mallet(double x, double y, Controller cont): Movable(x,y, 0.76, cont)
{
mass = 3; // 3kg
}

void Draw();
void Move();

};

class Puck: public Drawable, public Movable
{

public:

Puck(double x, double y): Movable(x,y, 0.7, physics) {
mass = 0.03; // 30g
}

void Draw();
void Move();

};

#endif

//----------------
//Derived.cpp - the functions for Derived.h
#include <vector>
#include <cmath>

#include "GLFiles.h"
#include "Derived.h"

#include "GeoMotion.h"

using namespace std;

//MalletXXX works when a mallet isnt hitting a XXX
void MalletWall(GeoMotion& coords); // if the object has hit a wall,
it returns the appropriate variable, else it returns the same variable
value
void MalletSquash(GeoMotion& coords, Mallet* mal); // has it hit a
squashes puck against the wall (for example)?
void MalletBound(GeoMotion& coords, Mallet* mal); // if its in your
half - y includes the position + change + radius

GeoMotion MouseCont(Mallet* mal); // mouse control
GeoMotion AICont(Mallet* mal); // AI control

bool Over(double coord, double fixed);

extern GLUquadricObj *quadratic;

extern vector<Drawable *> DrawList;
extern vector<Movable *> MoveList;

extern float length;
extern float width;
extern float rest;

extern int step; // to disect the mallets movement if its going into
another object that is unmovable

extern vector<Puck*> Pucks;

extern double friction;
extern int mouse_sens;

extern int WIDTH;
extern int HEIGHT;

void Puck:raw() {

glPushMatrix();

glColor3f(0.0f, 0.0f, 0.0f);

glTranslatef(x, 0.0f, y);
glRotatef(90, 1.0f, 0.0f, 0.0f);

gluCylinder(quadratic, radius, radius,0.1f,32,32); // main outside
gluDisk(quadratic,0.0f, radius,32,32); // disc covering top
gluCylinder(quadratic, 0.2, 0.2,0.11f,32,32); // nice looking ring in
the middle

glPopMatrix();

return;

}

void Puck::Move() {

x_change *= friction;
y_change *= friction;

x += x_change;
y += y_change;

return;

}

void Mallet:raw() {

double OuterHeight = 0.25; // outer mallet height
double WallDown = 0.1; // how far down the base of the handle is from
the outer height
double WallIn = 0.1; // how far in the outer wall comes
double HHeight = 0.4; // handle heigth
double HWidth = 0.6;

glPushMatrix();

glColor3f(0.0f, 0.0f, 0.0f);

glTranslatef(x, 0.0f, y);
glRotatef(-90, 1.0f, 0.0f, 0.0f);

gluCylinder(quadratic, radius, radius, OuterHeight ,32,32); //
outside wall

glTranslatef(0.0f, 0.0f, OuterHeight);
gluDisk(quadratic, radius-WallIn, radius,32,32); // top of the wall

glTranslatef(0.0f, 0.0f, -WallDown); // has been rotated, so z axis
is where y used to be, and -
gluCylinder(quadratic, HWidth/2 , radius-WallIn, WallDown,32,32); //
connection from handle to top of wall
gluCylinder(quadratic, HWidth/2 , HWidth/2, HHeight ,32,32); //
handle

glTranslatef(0.0f, 0.0f, HHeight);
gluSphere(quadratic, HWidth/2 ,32,32); // handle nob

glPopMatrix();

return;

}

void Mallet::Move() {

GeoMotion coords(this);

if (control == mouse)
coords = MouseCont(this);
else if (control == AI)
coords = AICont(this);

x = coords.x;
y = coords.y;

x_change = coords.x_change;
y_change = coords.y_change;

return;

}

GeoMotion MouseCont(Mallet* mal) {

GeoMotion coords(mal);

POINT mouse;
GetCursorPos(&mouse); // from windows.h

double tmpX = mouse.x - WIDTH/2; // centre screen gives large
coordinates, but coordinates in the centre should be 0,0 - without
this, centre coords will be like 512,384
double tmpY = mouse.y - HEIGHT/2;

coords.x_change = (tmpX / WIDTH * width)/mouse_sens; // get it as a
fraction of the screen, then make that fraction relative to the table
dimension, then divide by the sensitivity
coords.y_change = (tmpY / HEIGHT * length)/mouse_sens;

//continually send the coords to a function and have values updated
MalletWall(coords); // if its hitting a wall, return the change that
it should have to go max without hittin, else return current change
MalletBound(coords, mal);
MalletSquash(coords, mal);

coords.x = coords.x + coords.x_change; // make the x position its
curretn position plus its newly calculated change
coords.y = coords.y + coords.y_change;

SetCursorPos(WIDTH/2, HEIGHT/2);

return coords;

}

GeoMotion AICont(Mallet* mal) {

GeoMotion coords(mal);

coords.x_change = Pucks[0]->now_x() - coords.x;
coords.y_change=0;
MalletBound(coords,mal);
coords.x += coords.x_change;
coords.y += coords.y_change;

return coords;

}

void MalletBound(GeoMotion& coords, Mallet* mal) { // bound to own
half

double littlebit = 0.3;
double bound = mal->controller() == mouse? 0 + Pucks[0]->get_radius()
+littlebit : 0 - Pucks[0]->get_radius() -littlebit; // boundary,
furthest forward it can go. 0 is halfway

if (mal->controller() == mouse &&
coords.y+coords.y_change-coords.radius <= bound)
coords.y_change = bound - (coords.y-coords.radius);
else if ( mal->controller() != mouse && mal->controller() != physics
&& coords.y+coords.y_change+coords.radius >= bound)
coords.y_change = bound - (coords.y+coords.radius); // make distance
to be just touching

return;

}

void MalletSquash(GeoMotion& coords, Mallet* mal) {

for (int i=0; i<MoveList.size(); ++i) // if its being squashed
against a wall..
{
if (MoveList[i] != mal && MoveList[i]->controller() == physics) //
if its not the same address (in which case of course they will collide
and this will return false) and if its changeable on collision, like a
puck
if (sqrt( pow(coords.x+coords.x_change-MoveList[i]->next_x(), 2.0)
+ pow(coords.y+coords.y_change-MoveList[i]->next_y(), 2.0) ) <
coords.radius+MoveList[i]->get_radius()) // if (using distance
formula) its not hittign the object, if it is, their distance b/w
radii will be less than sum of radii
if ( Over(coords.x+coords.x_change, width/2-rest
-2*MoveList[i]->get_radius()-coords.radius) == true ||
Over(coords.y+coords.y_change, length/2-rest
-2*MoveList[i]->get_radius()-coords.radius) == true)
for (int j=0; j<=step; ++j)
if (sqrt( pow(coords.x+ j*coords.x_change/step
-MoveList[i]->next_x(), 2.0) + pow(coords.y+ j*coords.y_change/step
-MoveList[i]->next_y(), 2.0) ) <
coords.radius+MoveList[i]->get_radius())
{
coords.x_change = (j-1)*coords.x_change/step; // take the one
right before collision, otherwise the object will bounce back INSIDE
the mallet
coords.y_change = (j-1)*coords.y_change/step;
}
}

return;

}

void MalletWall(GeoMotion& coords) {

if (coords.x+coords.x_change + coords.radius > width/2-rest) // if
its going over the side wall
coords.x_change = width/2-rest -coords.radius - coords.x;

else if (coords.x+coords.x_change - coords.radius < -width/2+rest)
coords.x_change = -width/2+rest + coords.radius - coords.x; // make
it go right up to the wall

if (coords.y+coords.y_change + coords.radius > length/2-rest) // if
its going over the front/back wall
coords.y_change = length/2-rest -coords.radius - coords.y;

else if (coords.y+coords.y_change - coords.radius < -length/2+rest)
coords.y_change = -length/2+rest + coords.radius - coords.y; // make
it go right up to the wall

return;

}

void Movable::Replace() {

x_change = 0;
y_change = 0;

x = 0;

if (y<0) // if it needs to be reset on the other side
y = -length/4;
else
y = length/4;

double inity = y;

bool overlap;
do {
overlap = false;

for (int i=0; i< MoveList.size(); ++i)
if (MoveList[i] != this && sqrt( pow(y - MoveList[i]->y, 2.0) +
pow(x - MoveList[i]->x, 2.0) ) < radius + MoveList[i]->get_radius())
{
overlap = true;

x >= 0? x=-x-radius/2 : x=-x; // if its on the right, switch it to
the left, if its on the left sidemove it one object diameter across
and retry

if (x < -width/2+rest + radius) // dont keep replacing it, so
check its not off the edge, but make sure you dont place it too close,
at least a radius away
{
x = 0;

y = y >= inity? y - 2*(y-inity) -radius/2 : y + 2*(inity-y); //
switch sides each time, done by subtracting from y twice its distance
from length/4 (starting point) thereby reflecting it by this point

if (y < radius || y > -radius)
{
y=inity;
overlap = false; // we have placed it everywhere in the whole
half and nothing works, pretend there nothing wrong and set it in a
goal position to redo this procedure next iteration
}
}

break;
}
} while (overlap == true);

return;

}

void AddDraw(Drawable* draw) {
DrawList.push_back(draw);
return;
}

void AddMove(Movable* move) {
MoveList.push_back(move);
return;
}

//----------------
//DrawMotion.cpp - handles functions like drawing each frame,
collision detection, and how objects should reflect (both speed and
direction)
#include <vector>
#include <cmath>

#include "Derived.h"
#include "GLFiles.h"

using namespace std;

void SetupObj();
bool LoadPucks(int pucks);
bool LoadMallets();
void ReplacePucks();
void CloseGL();
void DrawObj();
bool Over(double coord, double fixed);
int CircleCol(Movable* obj1, Movable* obj2);
int FixedCol(Movable* obj, double x, double y);
double GoalCoord(double coord, double fixed);
void HitTable(Movable* obj, double change);
double NewVel(double vel1, double vel2, double m1, double m2);
void Collide();
void Reset();
void NextFrame();

extern Mallet* Mallet1;
extern Mallet* Mallet2;
extern vector<Puck*> Pucks;

extern float length;
extern float width;
extern float height;
extern float rest;
extern float goal;

extern int step;

vector<Drawable *> DrawList;
vector<Movable *> MoveList;

void SetupObj() {

ShowCursor(false);

if (LoadPucks(1) != true || LoadMallets() != true) // LoadPucks first
because mallet AI is used to check where the puck is and it will move
the puck first this way, and the AI can check
exit(-1);

ReplacePucks();

return;

}

bool LoadPucks(int pucks) {

Puck* tmpPuck;

for (int i=0; i<pucks; ++i)
{
try {
tmpPuck = new Puck(0,5);
Pucks.push_back(tmpPuck);
}
catch(std::bad_alloc nomem)
{
return false;
}
}

return true;

}

bool LoadMallets() {

try {
Mallet* Mallet1 = new Mallet(0,6, mouse);
Mallet* Mallet2 = new Mallet(0,1, AI);
}
catch(std::bad_alloc nomem)
{
return false;
}

return true;

}

void ReplacePucks() {

for (int i=0; i<Pucks.size(); ++i)
Pucks[0]->Replace();

return;

}

void CloseGL() {

ShowCursor(true);

delete Mallet1;
delete Mallet2;

for (int i=0; i<Pucks.size(); ++i)
delete Pucks[i];

return;

}

void DrawObj() {

for (int i=0; i < DrawList.size(); ++i)
DrawList[i]->Draw();

return;

}

bool Over(double coord, double fixed) { // has coord gone beyond fixed
in both positive and negative dimensions

if (coord > fixed || coord < -fixed)
return true;

return false;

}

int CircleCol(Movable* obj1, Movable* obj2) { // returns 0 if there no
collision, else it returns the step of the process where the collision
was

double xdist1 = obj1->now_x(); // its distance in the x direction
after any given number of steps through the process of deconstruction
double ydist1 = obj1->now_y();
double xdist2 = obj2->now_x();
double ydist2 = obj2->now_y();

double tmpX1 = obj1->change_x();
double tmpY1 = obj1->change_y();
double tmpX2 = obj2->change_x();
double tmpY2 = obj2->change_y();

for (int i=1; i<=step; ++i)
{
if (Over(xdist1 +tmpX1/step, width/2-rest -obj1->get_radius()) ==
true) // if any of the NEXT distances (thats why one step has been
added) are beyond a boundary, change the direction
tmpX1 *= -1;
if (Over(ydist1 +tmpY1/step, length/2-rest -obj1->get_radius()) ==
true && (xdist1+tmpX1/step >= goal/2 || xdist1+tmpX1/step <= -goal/2)
)
tmpY1 *= -1;
if (Over(xdist2 +tmpX2/step, width/2-rest -obj2->get_radius()) ==
true && (xdist2+tmpX2/step >= goal/2 || xdist2+tmpX2/step <= -goal/2)
)
tmpX2 *= -1;
if (Over(ydist2 +tmpY2/step, length/2-rest -obj2->get_radius()) ==
true)
tmpY2 *= -1;
// check this FIRST because the first step could step over a
boundary first and this needs to check it before ?dist is set to set
the correct direction of tmp?

xdist1 = obj1->now_x() + i * (tmpX1/step); // where it is + i
fractions of the change, i.e. more of the fraction
ydist1 = obj1->now_y() + i * (tmpY1/step);

xdist2 = obj2->now_x() + i * (tmpX2/step);
ydist2 = obj2->now_y() + i * (tmpY2/step);

if ( sqrt( pow(xdist1 - xdist2, 2.0) + pow(ydist1 - ydist2, 2.0) ) <
obj1->get_radius()+obj2->get_radius() ) // distance between radii <
the sum of the radii
return i;
}

return 0;

}

int FixedCol(Movable* obj, double x, double y) { // returns 0 if there
no collision, else it returns the step of the process where the
collision was

double xdist = obj->now_x(); // its distance in the x direction after
any given number of steps through the process of deconstruction
double ydist = obj->now_y();

double tmpX = obj->change_x();
double tmpY = obj->change_y();

for (int i=1; i<=step; ++i)
{
if (Over(xdist +tmpX/step, width/2-rest -obj->get_radius()) == true)
// if any of the NEXT distances (thats why one step has been added)
are beyond a boundary, change the direction
tmpX *= -1;
if (Over(ydist +tmpY/step, length/2-rest -obj->get_radius()) == true
&& (xdist+tmpX/step >= goal/2 || xdist+tmpX/step <= -goal/2) )
tmpY *= -1;
// check this FIRST because the first step could step over a
boundary first and this needs to check it before ?dist is set to set
the correct direction of tmp?

xdist = obj->now_x() + i * (tmpX/step); // where it is + i fractions
of the change, i.e. more of the fraction
ydist = obj->now_y() + i * (tmpY/step);

if ( sqrt( pow(xdist - x, 2.0) + pow(ydist - y, 2.0) ) <
obj->get_radius() ) // distance between radii < the sum of the radii
return i;
}

return 0;

}

double GoalCoord(double coord, double fixed) {

if (coord > 0)
return fixed;
else
return -fixed;

}

double NewVel(double vel1, double vel2, double m1, double m2) {
return vel1 * (m1-m2) / (m1+m2) + vel2 * 2 * m2 / (m1+m2);
}

void HitTable(Movable* obj, double change) {

if (Over(obj->next_y(), length/2-rest) && obj->next_x() <=
goal/2-obj->get_radius() && obj->next_x() >=
-goal/2+obj->get_radius()) // GOAL !! only reset its position when its
beyond saving
{
obj->Replace();
return; // no need to continue
}

if ( Over(obj->next_x(), width/2-rest - obj->radius) ) // If its hit
the side walls
obj->x_change *= -change;

if ( Over(obj->next_y(), length/2-rest - obj->radius) ) // possibly
hit the front/back wall
{
if ( obj->next_x() >= goal/2 || obj->next_x() <= -goal/2 ) //
hitting the actual wall
obj->y_change *= -change;

else // hitting the corner or its a goal
{
double gx = GoalCoord(obj->next_x(), goal/2); // goal x
double gy = GoalCoord(obj->next_y(), length/2-rest); // goal y

if ( int stepped = FixedCol(obj, gx, gy) ) // hits the corner of
the goal because its distance from it is less than its radius
{
double xdist = obj->x + stepped * obj->x_change/step;
double ydist = obj->y + stepped * obj->y_change/step;

double x = ( -obj->y_change - xdist*(ydist-gy)/(xdist-gx) +
(xdist+obj->x_change)*(gx-xdist)/(ydist-gy) ) / (
(gx-xdist)/(ydist-gy) - (ydist-gy)/(xdist-gx) ); // effect of the
corner (vector)
double y = ydist + x*(ydist-gy)/(xdist-gx) -
xdist*(ydist-gy)/(xdist-gx); // y effect

double invx = (xdist+obj->x_change) - x; // inverse
(perpendicular) velocities calculated before points are made relative
- otherwise the geometry is broken
double invy = (ydist+obj->y_change) - y;

x -= xdist;
y -= ydist;

obj->x_change = invx + change * -x; // corner just reverses the
motion (very solid)
obj->y_change = invy + change * -y;
}
}
}

return;

}

void Collide() {

double energy = 0.9; // _energy_ is multiplied by the velocities
after a collision compensating for energy loss to sound, heat etc.

int stepped;

for (int a=0; a < MoveList.size(); ++a)
for (int b=a+1; b < MoveList.size(); ++b) // only go through the
ones you havent already, because the first went through all, the
second need only go from the 3RD onwards
{
if ( stepped = CircleCol(MoveList[a], MoveList[b]) )
{
double x1; // x velocity contributed via centre of mass, i.e. the
vector that contributes to the change, this is the x coordinate
double y1;

double x2;
double y2;

double invx1; // x velocity NOT contributed via centre of mass,
i.e. the vector that compliments x1 making up the velocity
double invy1;

double invx2;
double invy2;

double xdist1 = MoveList[a]->x +
stepped*MoveList[a]->x_change/step;
double ydist1 = MoveList[a]->y +
stepped*MoveList[a]->y_change/step;

double xdist2 = MoveList[b]->x +
stepped*MoveList[b]->x_change/step;
double ydist2 = MoveList[b]->y +
stepped*MoveList[b]->y_change/step;

if (xdist1 == xdist2) // prevent division by 0
{
x1 = 0;
y1 = MoveList[a]->y_change;

invx1 = 0;
invy1 = 0;

x2 = 0;
y2 = MoveList[b]->y_change;

invx2 = 0;
invy2 = 0;
}
else if (ydist1 == ydist2)
{
x1 = MoveList[a]->x_change;
y1 = 0;

invx1 = 0;
invy1 = 0;

x2 = MoveList[b]->x_change;
y2 = 0;

invx2 = 0;
invy2 = 0;
}
else
{
x1 = ( (xdist1 +
MoveList[a]->x_change)*((xdist1-xdist2)/(ydist2-ydist1)) -
MoveList[a]->y_change - xdist1*((ydist2-ydist1)/(xdist2-xdist1)) ) / (
(xdist1-xdist2)/(ydist2-ydist1) - (ydist2-ydist1)/(xdist2-xdist1) );
y1 = (x1*(ydist2-ydist1)/(xdist2-xdist1)) - xdist1 *
(ydist2-ydist1)/(xdist2-xdist1) + ydist1;

invx1 = (xdist1+MoveList[a]->x_change) - x1;
invy1 = (ydist1+MoveList[a]->y_change) - y1;

x2 = ( (xdist2 +
MoveList[b]->x_change)*((xdist2-xdist1)/(ydist1-ydist2)) -
MoveList[b]->y_change - xdist2*((ydist1-ydist2)/(xdist1-xdist2)) ) / (
(xdist2-xdist1)/(ydist1-ydist2) - (ydist1-ydist2)/(xdist1-xdist2) );
y2 = (x2*(ydist1-ydist2)/(xdist1-xdist2)) - xdist2 *
(ydist1-ydist2)/(xdist1-xdist2) + ydist2;

invx2 = (xdist2+MoveList[b]->x_change) - x2;
invy2 = (ydist2+MoveList[b]->y_change) - y2;

x1 -= xdist1; // its co-ordinates are only relative to its
position, lets give them an absolute value by making them be the
amount of change, making where they were useless
y1 -= ydist1;

x2 -= xdist2;
y2 -= ydist2;
}

if (MoveList[a]->controller() == physics) // only change its
velocity if its an object thats manipulated like that
{
MoveList[a]->x_change = invx1 + energy * NewVel(x1,x2,
MoveList[a]->get_mass(), MoveList[b]->get_mass()); // we want to take
its initial velocity and change it (according to the masses) by the
impact of the 2nd object
MoveList[a]->y_change = invy1 + energy * NewVel(y1,y2,
MoveList[a]->get_mass(), MoveList[b]->get_mass());

HitTable(MoveList[a], 0); // if the object has been hit by
another object and hits the wall as well, its going to be in a fast
rebound for many iterations, but physically, the mallet would absorb
the energy, thus passing 0.0 so the velocity is nothing
}

if (MoveList[b]->controller() == physics)
{
MoveList[b]->x_change = invx2 + energy * NewVel(x2,x1,
MoveList[b]->get_mass(), MoveList[a]->get_mass());
MoveList[b]->y_change = invy2 + energy * NewVel(y2,y1,
MoveList[b]->get_mass(), MoveList[a]->get_mass());

HitTable(MoveList[b], 0);
}
}
}

for (int i=0; i<MoveList.size(); ++i)
if (MoveList[i]->controller() == physics)
HitTable(MoveList[i], energy);

return;

}

void Reset() {

double leeway = 0.02; // amount that the reset function allows the
puck to be away from the wall, this converts to 0.5cm

for (int i=0; i < MoveList.size(); ++i)
if (MoveList[i]->controller() == physics) // a puck
if ( Over( MoveList[i]->next_x(), width/2-rest -
MoveList[i]->get_radius() - leeway ))
if ( Over( MoveList[i]->next_y(), length/2-rest -
MoveList[i]->get_radius() - leeway )) // only check your side, cant
reset on the other side
if ( sqrt( pow(MoveList[i]->change_x(), 2.0) +
pow(MoveList[i]->change_y(), 2.0) ) < leeway) // if total velocity is
almost nothing
MoveList[i]->Replace();

return;
}

void NextFrame() {

Reset();
Collide();

for (int i=0; i < MoveList.size(); ++i)
MoveList[i]->Move();

return;

}

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
 
 
 
Victor Bazarov
Guest
Posts: n/a
 
      06-29-2003
I am not subscribed to c.l.c++.m, so I'm only replying in c.l.c++...

"wired" <(E-Mail Removed)> wrote...
> I've just taught myself C++,


I hope you don't see it as an event. "I've just spilled a bowl
of soup on my trousers" or something like that...

> so I haven't learnt much about style or
> the like from any single source, and I'm quite styleless as a result.


It is expected. Having learned rules of chess one hasn't become
a player yet.

> But at the same time, I really want nice code and I go to great
> lengths to restructure my code just to look concise and make it more
> manageable.
> When I say this, I'm also referring to the way I write my functions.
> It seems to me sometimes that I shouldn't have many void functions
> accepting addresses and changing their values for example, but rather
> a double function that returns the calculated value. i.e.
> void Change(Type& variable);
> should be replaced with
> Type Change(Type variable);


Some do think that a function, since it's name that, should only
work like a mathematical namesake: give a value based on input
parameters. That would introduce too much limitation, and even
the standard library is not written that way. Keep that in mind.

> I just don't know what's better code (in terms of speed, style, etc.).


Nobody knows. Style is a personal preference, speed is a feature
of a finished program. Yes, there are algorithms that serve the
same purpose and perform faster than others. Do use them if they
produce code the goal of which is as easy to recognise as with the
others. But don't concern yourself with speed too much before you
finish the functionality. First make it work, then make it fast.

You're right about making your code "maintainable" even if it means
maintenance by you. Some programmers produce enough code to easily
forget what they wrote a week before. They, like others would, go
back and look at their own code trying to figure out why certain
things were written in some way. So, for your own sake, do write
easily maintainable code.

> I'm moving into a more mature phase of my program, and before I
> continue I would like jsut a final verdict on my code.


Isn't this a bit too risky? I mean, using "final verdict" when
asking opinion on your code. If somebody says "it's crap", are
you going to give up programming? Hell, no. If somebody says,
"yeah, it'll work", are you going to stop improving yourself?

> I'm looking for
> comments about whether my functions should be more simple (i.e. too
> much happens in each), whether I should rather return value instead of
> alter through references passed, whether my general style is
> inelegant. I'm also concerned about my class structures. But I'll show
> you _some_ code and let you decide.
>
> Please note that I'm not asking you to go through the actual logic of
> all this code, but rather just calls to functions, classes and the
> like.


Davin,

I chose not to comment on any particulars of your code. It is
partly due to my laziness, and due to the fact that there are too
many things I'd change. The first couple headers produced at
least a dozen lines I'd type and that suggested to me that you
have still ways to go and perhaps your venture into "better style"
by asking advice is a tad premature.

Start by getting and reading good books. "Effective C++" series,
"C++ FAQ", and just plain "Accelerated C++" and even "TC++PL" are
good sources of information about style if you know what to look
for. You're asking good questions, try finding answers to them
in those books first. Hell, simply read through the C++ FAQ Lite
(you can find it here: http://www.parashift.com/c++-faq-lite/).

Try not to believe everything you read (my reply to your post is
no exception). However, if more than one person tells you to do
something, try to at least make a note of that.

Do write more code. Only by writing you will develop your style.
And don't let anybody tell you what style to have, whatever you
will develop should be good enough, but it doesn't have to be
a copy of anything.

Good luck!

Victor

P.S. Couldn't resist. Just a couple of pointers to improve your
code dramatically and immediately. (a) Use initialisation of
class members, not assignment in constructors. (b) Use at least
some amount of indentation in your code, making it easier to
understand. (c) If an arithmetic expression tends to span more
than one line of code, wrap it into a function. (d) If there is
even a shadow of a possibility that your lines are too long and
are going to be wrapped [by a news reading software at posting],
use /**/ comments instead of //; don't let your lines to have more
length than an average screen can hold. (e) Use better news posting
software, Google is notoriously bad. (f) Put your declarations
in headers, definitions in source modules, your cpp files should
never have to contain 'extern' unless it's 'extern "C"'... What
else is noticeable at the first glance? "using" directives in
a namespace scope are dangerous... "friend" declarations can be
omitted by better design... There is no need for "return" at
the end of a void function... Conditions of "blah == true" are
too verbose, they should be "blah"... Nah, I better stop now or
I will never stop


 
Reply With Quote
 
 
 
 
John Harrison
Guest
Posts: n/a
 
      06-29-2003

"wired" <(E-Mail Removed)> wrote in message
news:(E-Mail Removed) om...
> Hi,
>


[snip]

Victor's given you some good advice (with his usual brusque manner), here's
some comment on the actual code. Although as Victor said, don't believe
everything you read.

>
> Please note that I'm not asking you to go through the actual logic of
> all this code, but rather just calls to functions, classes and the
> like.


First point is the total absence of the keyword const. There are a zilllion
places you should add this, obviously in your self teaching you didn't come
across this concept, you should make it the very next thing you learn about.

>
> Thanks.
>
> Davin.
>
> //---------------------
> //Globals.cpp - just some global variables needed by most functions
> #include <vector>
> #include "GLFiles.h"
> #include "Derived.h"
>
> using std::vector;
>
> //quadratic to draw cylinders, spheres etc.
> GLUquadricObj *quadratic;


I'm always doubtful about global variables.

>
> //window dimensions
> int WIDTH = 1024;


const int WIDTH = 1024;

Not going to point out every variable that you should be declaring const.
Although global variables are dubious, global constants are fine. Also if
you make a variable const, you should move it into a header file.

> int HEIGHT = 768;
>
> //table dimensions
> float length = 25.15f; // 251.5cm
> float width = 13.84f;
> float height = 8.13f;
> float goal = 4.0f;
>
> float rest = 1.5f; // width between esdge and tabletop
>
> // Misc
>
> int step = 150; // number of steps the collision detection will
> deconstruct each translation into in checking for a collision
>
> Mallet* Mallet1;
> Mallet* Mallet2;
> vector <Puck*> Pucks;


More dubious globals, and I don't like pointers in STL classes, because it
makes memory management awkward. Since Puck is a polymorphic class I would
have used a smart pointer here. You can read about smart pointers in a good
book, 'More Effective C++' by Scott Meyers for instance.

>
> double friction = 0.985;
> int mouse_sens = 4; // 4-10, 10-least sensitive
>
> //---------------------
> //Abstract.h - two abstract classes, some derived classes will be
> inherited from both
> #ifndef Abstract_Classes
> #define Abstract_Classes
>
> #include "Globals.h"
>
> using namespace std;
>
> class Movable;
> class Drawable;
>
> void AddDraw(Drawable* draw);
> void AddMove(Movable* move);
>
> extern double friction;
>
> class Drawable
> {
>
> public:
>
> Drawable() {
> AddDraw(this);
> }
>
> virtual void Draw() = 0;


As a classes which is designed for derivation Drawable must have a virtual
destructor other wise ou get undefined behaviour when you try to delete an
object of a class derived from Drawable using a pointer to Drawable.

virtual ~Drawable() {}

>
> };
>
> class Movable
> {
>
> public:
>
> Movable(double x1, double y1, double r, Controller cont) {


Not sure what Controller is but consider a const reference here for
efficiency. And definitely prefer initialiation to assignment

Movable(double x1, double y1, double r, const Controller& cont) :
control(cont), x(x1), y(y1), radius(r), x_change(0.0), y_change(0.0) {

>
> AddMove(this);
>
> control = cont;
>
> x = x1;
> y = y1;
>
> x_change = 0.0;
> y_change = 0.0;
>
> radius = r;
>
> }


Ditto, virtual destructor needed.

virtual ~Movable() {}

>
> double next_x() { return x + friction * x_change; }


Should be const as should most of the other methods.

double next_x() const { return x + friction * x_change; }

> double next_y() { return y + friction * y_change; }
>


[snip]

OK, I'm bored to now. You do look like a self taught programmer. You've got
some concepts spot on, and others you've completely missed out on. You need
a good book, and practise.

john


 
Reply With Quote
 
E. Robert Tisdale
Guest
Posts: n/a
 
      06-29-2003
wired wrote:
> It seems to me sometimes that I shouldn't have many void functions
> accepting addresses and changing their values for example
> but, rather, a double function that returns the calculated value. i.e.
>
> void Modify(Type& variable);
>
> should be replaced with
>
> Type Transform(Type variable);


or

Type Transform(const Type& variable);

> I just don't know what's better code (in terms of speed, style, etc.).


Sometimes, you must modify objects "in-place" so you would implement

Type& Modify(Type& variable);

where the function modifies variable in-place then returns a reference
to variable so that your Modify function can be used in expressions
that may also modify variable. For example,
an object which is an instance of one of the container classes
is almost always a variable which requires modification.

Otherwise, whether you pass function arguments
by value or by const reference usually depends
only upon the size of the object.
If the object occupies no more than one or two machine words,
it is probably faster and more efficient to pass by value.
Larger objects are almost always passed by const reference.

> cat f.h

#include<iostream>

class X {
private:
// representation
int I;
public:
// operators
operator int(void) const { return I; }
X(int i): I(i) { }
};

X f(X);
X f(const X&);

> cat f.cc

#include<f.h>

X f(X x) {
std::cout << "X f(X x)" << std::endl;
return (int)x + 33;
}

X f(const X& x) {
std::cout << "X f(const X& x)" << std::endl;
return (int)x + 33;
}

> cat main.cc

#include<f.h>

int main(int argc, char* argv[]) {
X x = 22;
X y = f(x);
std::cout << (int)y << " = f(x)" << std::endl;
return 0;
}

> g++ -Wall -ansi -pedantic -I. -O2 -o main main.cc f.cc

main.cc: In function `int main(int, char**)':
main.cc:5: call of overloaded `f(X&)' is ambiguous
f.h:13: candidates are: X f(X)
f.h:14: X f(const X&)

The nice thing about this is that you can substitute
function f(const X&) for function f(X) and vice versa
without affecting the meaning of the application program
just as long as function f doesn't modify any global variable
that can be passed to it in its argument list.


[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
wired
Guest
Posts: n/a
 
      06-30-2003
Hi,

Thanks heaps! The response has been fantastic, and the advice endless
(for good reason). Sorry about the lack of indentation, in my code its
there, and through the copy-paste it must have squeezed itself - as
suggested I should think about a proper newsreader.

Its good to get back consistent feedback (const use, globals,
namespaces etc.), incredibly settling!

So once again thanks for your help.

Any further posts I'll be sure to read though, so don't stop posting
because I posted this message

Davin.

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
Francis Glassborow
Guest
Posts: n/a
 
      06-30-2003
In message <(E-Mail Removed)>, E. Robert Tisdale
<(E-Mail Removed)> writes
>> 1) Functions that are designed for doing (e.g. display a graphic)
>> should return void or, if member functions possibly *this.

>
>Nonsense!
>There is seldom a good reason to define a void function.
>(A destructor, for example, is an exception to this rule.)
>At the very least, a function should return a reference
>(or, perhaps, a pointer) to the object that was modified
>so that the function can be used in expressions.


You are entitled to an opinion but it is not widely shared by expert C++
programmers (such as Bjarne Stroustrup, Nico Josuttis, Scott Meyers, Rob
Murray, Steve Dewhurst, Herb Sutter, Andrei Alexandrescu ...). The
significance of a function returning void in C++ is that it is really a
procedure which is a well understood concept in computer science.
And destructors simply do not have return values of any kind so they
are not exceptions to a guideline, the programmer is never given the
choice.

What should be the return type of:

??? foo(){
std::cout << std::clock();
}

--
ACCU Spring Conference 2003 April 2-5
The Conference you should not have missed
ACCU Spring Conference 2004 Late April
Francis Glassborow ACCU


[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
kanze@gabi-soft.fr
Guest
Posts: n/a
 
      06-30-2003
Francis Glassborow <(E-Mail Removed)> wrote in message
news:<hJntmnHBQs$(E-Mail Removed)>...
> In message <(E-Mail Removed) >, wired
> <(E-Mail Removed)> writes


> >Please note that I'm not asking you to go through the actual logic of
> >all this code, but rather just calls to functions, classes and the
> >like.


> A few possible guidelines:


All excellent. Just one further comment...

> 4) Think very carefully about deriving from a concrete class


> 5) Think even more carefully about multiple inheritance from concrete
> classes. Mostly MI works well for adding interfaces.


I think that these two rules can be subsumed by a more general rule:
think in terms of patterns, and what pattern(s) your inheritance
hierarchy uses. IMHO, there is nothing per se really wrong about
deriving from a concrete class, even with multiple inhertance. It's
just that there are remarkably few patterns that do it, so that most of
the time, it tends to be a symptom of class design by carelessly
throwing various functionality together.

--
James Kanze GABI Software (E-Mail Removed)
Conseils en informatique orientée objet/ http://www.gabi-soft.fr
Beratung in objektorientierter Datenverarbeitung
11 rue de Rambouillet, 78460 Chevreuse, France, Tél. : +33 (0)1 30 23 45 16

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
Thomas Mang
Guest
Posts: n/a
 
      06-30-2003


"E. Robert Tisdale" schrieb:

> Otherwise, whether you pass function arguments
> by value or by const reference usually depends
> only upon the size of the object.
> If the object occupies no more than one or two machine words,
> it is probably faster and more efficient to pass by value.
> Larger objects are almost always passed by const reference.


Really? That's news to me.

Here is a very simple String class, with size most likely to be 4 on a 32
bit machine:

class String
{
public:
String(const char* Text) : text_(new char[std::strlen(Text) + 1])
{std::strcpy(text_, Text);}
~String() {delete [] text_;}
String(const String& rhs) : text_(new char[std::strlen(rhs.text_) + 1])
{std::strcpy(text_, rhs.text_);}
// assignment operator, .....

private:
char* text_;
};

Would you pass such a class around by value?
I wouldn't, because the copy constructor is expensive.

OTOH, larger classes with a cheap copy constructor (e.g. one that copies its
members by bitwise copying) won't hurt much when passed-by-value.

Just looking at what sizeof() returns and deciding on that number whether to
pass-by-value or -by-reference is, at least IMHO, not the way to go.


regards,

Thomas

regards,

Thomas

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
Victor Bazarov
Guest
Posts: n/a
 
      06-30-2003
"Francis Glassborow" <(E-Mail Removed)> wrote...
> In message <(E-Mail Removed)>, E. Robert Tisdale
> <(E-Mail Removed)> writes
> >> 1) Functions that are designed for doing (e.g. display a graphic)
> >> should return void or, if member functions possibly *this.

> >
> >Nonsense!
> >There is seldom a good reason to define a void function.
> >(A destructor, for example, is an exception to this rule.)
> >At the very least, a function should return a reference
> >(or, perhaps, a pointer) to the object that was modified
> >so that the function can be used in expressions.

>
> You are entitled to an opinion but it is not widely shared by expert

C++
> programmers (such as Bjarne Stroustrup, Nico Josuttis, Scott Meyers,

Rob
> Murray, Steve Dewhurst, Herb Sutter, Andrei Alexandrescu ...). The
> significance of a function returning void in C++ is that it is really

a
> procedure which is a well understood concept in computer science.
> And destructors simply do not have return values of any kind so they
> are not exceptions to a guideline, the programmer is never given the
> choice.
>
> What should be the return type of:
>
> ??? foo(){
> std::cout << std::clock();
> }


'bool' or 'iostream&' so that the caller could verify the completeness
of the operation. The code should probably have

return std::cout.good();

or

return std::cout;

in it.

But that's more or less irrelevant. Your argument about 'void'
functions' right to exist is valid. You just didn't use a good
example.

Victor



[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
Jerry Coffin
Guest
Posts: n/a
 
      07-01-2003
In article <(E-Mail Removed) >, lunz008
@hotmail.com says...

[ ... ]

A few more comments on things I haven't seen anybody else mention yet.

> //Abstract.h - two abstract classes, some derived classes will be
> inherited from both
> #ifndef Abstract_Classes
> #define Abstract_Classes
>
> #include "Globals.h"
>
> using namespace std;


A using declaration like this in a header should normally be avoided.

[ ... ]

> class Movable
> {

[ ... ]

> double get_radius() { return radius; }
> double get_mass() { return mass; }


So everything that's movable has a radius and a mass? That sounds a
little questionable to me...

[ ... ]

> class Mallet: public Drawable, public Movable

[ ... ]
> class Puck: public Drawable, public Movable


Are there objects that are drawable and not movable, or vice versa?
Right now, it looks like all actual objects are both drawable and
movable, in which case putting those into separate classes accomplishes
nothing useful.

In Mallet::Move, you have an explicit check for whether the controller
is the AIController, or the MouseController, and then you get the
coordinates from one or the other. This is almost certainly a poor
idea. I'd use something like:

Controller {
public:
virtual GeoMotion getCoords() = 0;
};

class MouseCont : public Controller {
public:
GeoMotion getCoords {
// ...
}
};

class AICont : public Controller {
public:
GeoMotion getCoords {
// ...
}
};

Then
> void Mallet::Move() {
>
> GeoMotion coords(this);
>
> if (control == mouse)
> coords = MouseCont(this);
> else if (control == AI)
> coords = AICont(this);


becomes:

void Mallet::Move() {

GeoMotion coords(this);

cords = Controller.getCoords();

> x = coords.x;
> y = coords.y;


If you're going to define a coords class, I'd try to use it wherever
applicable. E.g.:

coords current_pos;

in which case, you'd just use:

current_pos = Controller.getCoords();

> x_change = coords.x_change;
> y_change = coords.y_change;


It seems highly suspect to me to have something called "coords" that
apparently only specifies coordinates, but also a movement vector.

> if (y<0) // if it needs to be reset on the other side
> y = -length/4;
> else
> y = length/4;


y = fabs(length/4);

[ ... ]

> x >= 0? x=-x-radius/2 : x=-x;


This isn't really how the conditional is intended to be used. The
intent is more like:

x = x>=0 ? x-radius/2 : -x;

[ ... ]

> } while (overlap == true);


As a rule, comparing anything to true is a poor idea. I'd just use:

} while (overlap);

[ ... ]

> if (LoadPucks(1) != true || LoadMallets() != true) // LoadPucks first


Likewise, here I'd just use:
if ( !loadPucks(1) || !LoadMallets())

> try {
> tmpPuck = new Puck(0,5);
> Pucks.push_back(tmpPuck);
> }
> catch(std::bad_alloc nomem)
> {
> return false;
> }


IMO, this is a poor idea. It would probably be better to use exception
handling throughout, so this part of the code would just let the
exception propagate to somewhere else that it could really be handled.

[ ... ]

> bool Over(double coord, double fixed) { // has coord gone beyond fixed
> in both positive and negative dimensions
>
> if (coord > fixed || coord < -fixed)
> return true;
>
> return false;


return coord > fixed || coord < - fixed;

> void Collide() {
>
> double energy = 0.9; // _energy_ is multiplied by the velocities
> after a collision compensating for energy loss to sound, heat etc.


IMO, most things like this should normally be in external data files.
This makes it much easier to do things like defining objects with
different characteristics (e.g. hard pucks vs. soft, squishy pucks).

I'd also define this to receive a couple of iterators defining the
collisions to check, rather than always operating on the global
MoveList.

On a more general level, your code is composed mostly of a few large
classes and a few large functions. I'd try to break things up a bit
more, into discrete chunks that are smaller and easier to understand
individually. Just for a really obvious example, you have the usual
Pythagorean distance formula in a LOT of different places. The code
would be much easier to understand if you defined a function for this,
and then used it elsewhere. In addition, a few temporary variables can
help readability dramatically. eg.:

float dist(int x, int y) {
return sqrt(x * x + y * y);
}

// If I'm following this correctly, this loop can be replaced
// with some geometry and end up faster and more accurate.
void find_step(GeoMotion &c, Moveable const &m) {
for (int i=0; i<step; ++i) {
float x_step = c.x+i*c.x_change/step;
float y_step = c.y+i+c.y_change/step;

if (dist(x_step, y_step) < c.radius + m.get_radius()) {
c.x_change = (j-1)*c.x_change/step;
x.y_change = (j-1)*c.y_change/step;
// guessing at an addition:
break;
}
}
}

void MalletSquash(GeoMotion &coords, Mallet *mal) {
for (int i=0; i<MoveList.size(); ++i) {
Moveable &m = *MoveList[i];
if ( &m != mal && m.controller() == physics) {
double next_x = coords.x+coords.x_change;
double next_y = coords.y+coords.y_change;
double &r = coords.radius;

if ( dist(next_x, next_y) < r + m.get_radius())
if (Over(next_x, ...) || Over(next_y, ...)
find_step(coords, m);
}
}
}

and we're at least getting a bit closer to something readable. I doubt
it's optimal yet, but I'm not sure I followed what was going on well
enough to suggest many further improvements.

--
Later,
Jerry.

The universe is a figment of its own imagination.

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Cleaning Up, Mucking Out and Paranoid Programming aarklon@gmail.com C Programming 1 04-10-2008 06:45 AM
US Postal Service (Am I paranoid?) meerkat Computer Support 39 12-09-2007 08:37 AM
REVIEW: "PGP & GPG: Email for the Practical Paranoid", Michael W. Lucas Rob Slade, doting grandpa of Ryan and Trevor Computer Security 0 10-09-2006 05:17 PM
Paranoid about Warranties ? Magnusfarce Digital Photography 5 10-10-2004 05:07 PM
Outlook Express is Paranoid! Howard ALEXANDER Computer Support 12 09-30-2003 10:10 PM



Advertisments