Paranoid about style/elegance and function size, please quell

[wired]

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 != mal && MoveList->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->next_x(), 2.0)
+ pow(coords.y+coords.y_change-MoveList->next_y(), 2.0) ) <
coords.radius+MoveList->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->get_radius()-coords.radius) == true ||
Over(coords.y+coords.y_change, length/2-rest
-2*MoveList->get_radius()-coords.radius) == true)
for (int j=0; j<=step; ++j)
if (sqrt( pow(coords.x+ j*coords.x_change/step
-MoveList->next_x(), 2.0) + pow(coords.y+ j*coords.y_change/step
-MoveList->next_y(), 2.0) ) <
coords.radius+MoveList->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 != this && sqrt( pow(y - MoveList->y, 2.0) +
pow(x - MoveList->x, 2.0) ) < radius + MoveList->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;

return;

}

void DrawObj() {

for (int i=0; i < DrawList.size(); ++i)
DrawList->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) )
{
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->x +
stepped*MoveList->x_change/step;
double ydist2 = MoveList->y +
stepped*MoveList->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->y_change;

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

invx1 = 0;
invy1 = 0;

x2 = MoveList->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->x_change)*((xdist2-xdist1)/(ydist1-ydist2)) -
MoveList->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->x_change) - x2;
invy2 = (ydist2+MoveList->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->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->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->controller() == physics)
{
MoveList->x_change = invx2 + energy * NewVel(x2,x1,
MoveList->get_mass(), MoveList[a]->get_mass());
MoveList->y_change = invy2 + energy * NewVel(y2,y1,
MoveList->get_mass(), MoveList[a]->get_mass());

HitTable(MoveList, 0);
}
}
}

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

return;
}

void NextFrame() {

Reset();
Collide();

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

return;

}

 

[Victor Bazarov]

I am not subscribed to c.l.c++.m, so I'm only replying in c.l.c++...

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

 

[John Harrison]

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

 

[E. Robert Tisdale]

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.

 

[wired]

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.

 

[Francis Glassborow]

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();
}

 

[kanze]

In message <[email protected]>, wired

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.

 

[Thomas Mang]

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

 

[Victor Bazarov]

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

 

[Jerry Coffin]

[ ... ]

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;
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.

 

[E. Robert Tisdale]

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 ...).

Please cite the references and quote the relevant passages.

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.

It is well understood that procedural programs
are difficult read, understand and maintain.
Each procedure call changes the state of the program.
The state affects the flow control
which can change the thread of execution.
The thread of execution determines the sequence of procedure calls, etc.
What you end up with is spaghetti.

Procedural programs are difficult to analyze
and practically impossible to prove correct.
You should avoid procedure calls (and assignment statements)
whenever possible.

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:

std:stream& foo(std:stream& os){
std:s << std::clock();
return os;
}

At the very least, a function should return a reference
to the object that was modified
so that the function can be used in expressions.

cat Francis.cc

#include<iostream>

std:stream& foo(std:stream& os){
os << std::clock();
return os;
}

int
main(int argc, char* argv[]) {
foo(std::cout) << std:: endl;
return 0;
}

 

[Ed Avis]

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
...).

You can't just drop a long list of names without giving some
references to where we can find their opinions to back you up. And if
giving references for each of them would be too long-winded, just
enlist fewer of them in your support.

I imagine that the behaviour of C++'s assignment operator - which
returns a reference to the object that was modified, just as the
previous poster suggested - indicates that B. Stroustrup prefers that
style rather than the 'pure procedural' style where modification
operations cannot be changed.

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.

What exactly is this concept? If you say that 'a procedure does not
return a value' then that is tautologically true, but it doesn't give
any reason to write programs in this style.

What should be the return type of:

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

This doesn't fit the case proposed for having a returned value, which
is when the value of an object is modified. operator<<() isn't really
a value-modification operation.

Still, you might like to consider what is the return type of
operator()<< itself.

 

[Sean Fraley]

It is well understood that procedural programs
are difficult read, understand and maintain.
Each procedure call changes the state of the program.
The state affects the flow control
which can change the thread of execution.
The thread of execution determines the sequence of procedure calls, etc.
What you end up with is spaghetti.

He didn't say that a "prodedural program" is a well understood concept in
computer science. He said that a "procedure" is a well understood concept
in computer science. He is talking about a logical concept, no an example
of a particular design paradigm.

Procedural programs are difficult to analyze
and practically impossible to prove correct.

The problem may lay with the skills of the person doing the analysis, not
the fact that the program is procedural.

You should avoid procedure calls (and assignment statements)
whenever possible.

Why? I can possibly see this from some academically pure point of view, but
in the real world, writing real programs that are used to solve real
problems, this statement seems rather inane. If you're going to make a
broad doctrinal statement like "avoid procedural calls whenever possible",
then at least provide some rationale as to why. The person who started
this thread is someone who is looking for input and ideas to help them
progress as a programmer. We want up and coming programmers to know WHY
something is done, so that way they can make an informed decision as to
WHAT they should do.

At the very least, a function should return a reference
to the object that was modified
so that the function can be used in expressions.

Once again, why? In the example that you give above, you've not really
accomplished anything useful. The operator<< functions of std:stream
already return a reference to the ostream. If I want to insert the return
value of std::clock into an ostream, then

std::cout << "this program has run for " << std::clock()
<< " clock ticks.\n";

is actually a better choice than

std::cout << "this program has run for "
<< foo(std::cout)
<< "clock ticks.\n";

for the following reasons:

1) it works like you think it should. Try writing, compiling, and then
running some code with your implementation of foo. It does some weird
stuff.

2) you don't have to pass a parameter to foo, thereby reducing the
requirements of code that is a client of foo.

3) the original implementations purpose is immediately apparent at even a
slight glance, and a client with access to nothing but the prototype in the
header sees nothing confusing about it. With you're implementation, the
first question to be asked by a client is "why the f*** should I have to
pass an ostream as a parameter just to print the number of clock ticks
since start to standard output? That makes no sense."

he original implementation makes since in terms of the design of the
program. If the specification says "a function to print the number of
clock ticks since start to standard output." Then a void function that
takes no arguments is just what you need.

This is why I have a real problem with purely academic doctrine being thrown
around like it is some kind of time tested wisdom. In order to change

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

to fit into your little rule of "always return something, even if it's a
reference to the object being modified.", you had to remove most of what
made it useful. The original implementation makes since in terms of the
design of the program. If the specification says "a function to print the
number of clock ticks since start to standard output." Then a void
function that takes no arguments is just what you need. Drastically
changing it's argument list, not to mention implementing it in such a way
that it behaves badly at runtime, for no other reason than the slim chance
that someone might want to use it in an expression at some point in the
future, is a bad decision.

 

[Francis Glassborow]

You can't just drop a long list of names without giving some
references to where we can find their opinions to back you up. And if
giving references for each of them would be too long-winded, just
enlist fewer of them in your support.

You miss the point. Please name a single respectable C++ writer who
follows Mr Tisdale's dictum.

I imagine that the behaviour of C++'s assignment operator - which
returns a reference to the object that was modified, just as the
previous poster suggested - indicates that B. Stroustrup prefers that
style rather than the 'pure procedural' style where modification
operations cannot be changed.

Let me reinsert the context:

1) Functions that are designed for doing (e.g. display a graphic) should
return void or, if member functions possibly *this.

Which was one of my guidelines wrt to writing code. Now go and examine
the writings of any of the above list of authors and you will find that
they implicitly apply that guideline in their published code (i.e. you
will find them using a void return type whenever there is no sensible
value to return.

Bjarne Stroustrup was/is following a policy of as close to C... so he
had very little choice about operators including the assignment ones in

C++ yielding values. I suspect that Ritchie may have been heavily
influenced by the precursors to C (such as B and BCPL). In practice
there are numerous bugs in code that are a result of the C and C++
assignment operators returning values.
And where did I mention a pure procedural style? I would like some
support for a pure functions but that is a different issue.

It is also interesting to note that relatively late on C++ increased its

support for 'procedure' type functions by allowing return of the call of

a void function to a function with a void return.

 

[John Potter]

std::cout << "this program has run for " << std::clock()
<< " clock ticks.\n";

is actually a better choice than

std::cout << "this program has run for "
<< foo(std::cout)
<< "clock ticks.\n";

for the following reasons:

1) it works like you think it should. Try writing, compiling, and then
running some code with your implementation of foo. It does some weird
stuff.

Be nice. Do not misuse it. Use it properly and complain about that.

foo(std::cout << "this program has run for ")
<< "clock ticks.\n";

John

 

[E. Robert Tisdale]

Be nice. Do not misuse it. Use it properly and complain about that.

foo(std::cout << "this program has run for ")
<< "clock ticks." << std::endl;

I hope everyone remembers that Sean is complaining about
Francis Glassborow's function and not mine.
I felt that I was obliged to do the best I could
with the function that Francis gave us.

 

[E. Robert Tisdale]

Let me reinsert the context:

1) Functions that are designed for doing (e.g. display a graphic) should
return void or, if member functions possibly *this.

Which was one of my guidelines wrt to writing code. Now go and examine
the writings of any of the above list of authors and you will find that
they implicitly apply that guideline in their published code (i.e. you
will find them using a void return type whenever there is no sensible
value to return.

Bjarne Stroustrup was/is following a policy of as close to C... so he
had very little choice about operators including the assignment ones in

C++ yielding values. I suspect that Ritchie may have been heavily
influenced by the precursors to C (such as B and BCPL). In practice
there are numerous bugs in code that are a result of the C and C++
assignment operators returning values.
And where did I mention a pure procedural style? I would like some
support for a pure functions but that is a different issue.

It is also interesting to note that relatively late on C++ increased its

support for 'procedure' type functions by allowing return of the call of

a void function to a function with a void return.

According to Brian W. Kernighan and Dennis M. Ritchie,
"The C Programming Language", Copyright 1978,
Chapter 6 Structures, Section 2 Structures and Functions, page 121:

"There are a number of restrictions on C structures.
The essential rules are that the only operations
that you can perform on a structure are take its address with &,
and access on of its members. This implies that
structures may not be assigned to or copied as a unit,
and that they can not be passed to or returned from functions.
(These restrictions will be removed in forth-coming versions.)"

Appendix A C Reference Manual, Section 14 Types revisited,
Subsection 1 Structures and unions, page 209:

"There are only two things that can be done with structures and unions:
name one of its members (by means of the . operator);
or take its address (by unary &). Other operations, such as assigning
from or to it or passing it as a parameter, draw an error message."

These restrictions on passing and returning struct's
have long since been removed from the C computer programming language
but old habits die hard -- especially bad habits.
There is no longer any reason to pass a pointer to the return value
explicitly in the function argument list.
Your compiler will do that for you.
It is still a good idea to pass a const pointer
if you need to reference large objects from the function.

 

[Albrecht Fritzsche]

You can't just drop a long list of names without giving some
references to where we can find their opinions to back you up.

It is really not that hard to find some articles by those mentioned
above. Google sometimes does wonders, but

www.cuj.com/experts

might be a sufficient starting point if you haven't have any books
by those authors handy or not read yet.

What exactly is this concept? If you say that 'a procedure does not
return a value' then ... it doesn't give
any reason to write programs in this style.

Of course not, but if you head for a clear and concise(?) style than
you might find procedures sometimes the most valuable tool. Just
have a look at the STL - there those procedures are called algorithms
and more often than not having a void return.

Until now noone has complained, though.
Ali

 

[John Potter]

1) Functions that are designed for doing (e.g. display a graphic) should
return void or, if member functions possibly *this.

ostream& operator<< (ostream&, string const&);

Which was one of my guidelines wrt to writing code. Now go and examine
the writings of any of the above list of authors and you will find that
they implicitly apply that guideline in their published code (i.e. you
will find them using a void return type whenever there is no sensible
value to return.

Your statement is acceptable, but not your intent. Whenever there is
any way to use the imagination to create a sensible return, the function
is not void. Void is a last resort used in inverse proportion to the
sensibleness of the user.

John

 

[Francis Glassborow]

Please cite the references and quote the relevant passages.

Please read clause 25, Algorithms library of the C++ standard. It is too
long to quote here but you should find a copy in JPL's reference
library.
Those wishing to avoid using procedures (functions returning nothing)
must, effectively, avoid the C++ library and any third party library
that does not guarantee to avoid all standard functions that return
void.

It is well understood that procedural programs
are difficult read, understand and maintain.

I was not writing about procedural programs but that certain actions are
inherently procedures (i.e. are used for their side effects). Even
functional programming languages such as Haskell accept that some
procedural type actions are unavoidable though they try to minimise
them.

Each procedure call changes the state of the program.

But that is just a subset, each call of a function that has side effects
potentially changes the state of the program. A function in C++ can only
return one thing, which means that any function that is (in)directly
responsible for changing two or more non-local objects has to choose
which to return if either.

The state affects the flow control
which can change the thread of execution.
The thread of execution determines the sequence of procedure calls, etc.
What you end up with is spaghetti.

If we try to use uncontrolled procedural programming, but that is very
different from using procedures to carry out actions and functions to
evaluate and return values. Unfortunately (in the opinion of purists)
C++ supports everything from pure procedures to pure functions with
every conceivable mixture in between. At least I can call out my
procedures by giving them a void return type. I wish I had a similar
mechanism to identify pure functions.

Procedural programs are difficult to analyze
and practically impossible to prove correct.

I never advocated writing pure procedural programs.

You should avoid procedure calls (and assignment statements)
whenever possible.

Then C++ is a bad language to use, Haskell would be a much better
choice.

That is not what I wrote, please be more careful about attributions. Had
I wanted such a function I would have written:

std:stream& foo(std:stream & out){
return out<< std::clock();
}

or even:

bool foo(std:stream & out)
return not (out << std::clock()).fail();
}

Please note that your code includes a non-existent object (std:s) and
so should fail to compile.