Paranoid about style/elegance and function size, please quell

Discussion in 'C++' started by wired, Jun 29, 2003.

  1. wired

    wired Guest

    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::Draw() {

    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::Draw() {

    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[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 [url]http://www.gotw.ca/resources/clcm.htm[/url] for info about ]
    [ comp.lang.c++.moderated. First time posters: Do this! ][/i][/i][/i][/i][/i][/i][/i][/i][/i][/i][/i][/b][/b][/b][/b][/b][/b][/b][/b][/b][/b][/b][/b][/b][/b][/b]
    wired, Jun 29, 2003
    #1
    1. Advertising

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

    "wired" <> 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 :)
    Victor Bazarov, Jun 29, 2003
    #2
    1. Advertising

  3. "wired" <> wrote in message
    news:...
    > 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
    John Harrison, Jun 29, 2003
    #3
  4. 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! ]
    E. Robert Tisdale, Jun 29, 2003
    #4
  5. wired

    wired Guest

    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! ]
    wired, Jun 30, 2003
    #5
  6. In message <>, E. Robert Tisdale
    <> 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! ]
    Francis Glassborow, Jun 30, 2003
    #6
  7. wired

    Guest

    Francis Glassborow <> wrote in message
    news:<hJntmnHBQs$>...
    > In message <>, wired
    > <> 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 mailto:
    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! ]
    , Jun 30, 2003
    #7
  8. wired

    Thomas Mang Guest

    "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! ]
    Thomas Mang, Jun 30, 2003
    #8
  9. "Francis Glassborow" <> wrote...
    > In message <>, E. Robert Tisdale
    > <> 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! ]
    Victor Bazarov, Jun 30, 2003
    #9
  10. wired

    Jerry Coffin Guest

    In article <>, 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;
    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! ]
    Jerry Coffin, Jul 1, 2003
    #10
  11. Francis Glassborow wrote:
    > In message <>, E. Robert Tisdale
    > <> 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 ...).


    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::eek:stream& foo(std::eek:stream& os){
    > std::eek: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::eek:stream& foo(std::eek:stream& os){
    os << std::clock();
    return os;
    }

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

    > g++ -Wall -ansi -pedantic -O2 -o Francis Francis.cc



    [ See http://www.gotw.ca/resources/clcm.htm for info about ]
    [ comp.lang.c++.moderated. First time posters: Do this! ]
    E. Robert Tisdale, Jul 1, 2003
    #11
  12. wired

    Ed Avis Guest

    Francis Glassborow <> writes:

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


    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.

    --
    Ed Avis <>

    [ See http://www.gotw.ca/resources/clcm.htm for info about ]
    [ comp.lang.c++.moderated. First time posters: Do this! ]
    Ed Avis, Jul 1, 2003
    #12
  13. wired

    Sean Fraley Guest

    E. Robert Tisdale wrote:

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


    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.

    > > 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::eek:stream& foo(std::eek:stream& os){
    > > std::eek: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.


    Once again, why? In the example that you give above, you've not really
    accomplished anything useful. The operator<< functions of std::eek: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.

    [ See http://www.gotw.ca/resources/clcm.htm for info about ]
    [ comp.lang.c++.moderated. First time posters: Do this! ]
    Sean Fraley, Jul 1, 2003
    #13
  14. In message <-i.net>, Ed Avis
    <> writes
    >Francis Glassborow <> writes:
    >
    >>>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
    >>...).

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

    --
    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! ]
    Francis Glassborow, Jul 1, 2003
    #14
  15. wired

    John Potter Guest

    On 1 Jul 2003 14:21:43 -0400, Sean Fraley <>
    wrote:

    > 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
    John Potter, Jul 1, 2003
    #15
  16. John Potter wrote:

    > Sean Fraley wrote:
    >
    >> 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." << 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, Jul 2, 2003
    #16
  17. Francis Glassborow wrote:

    > 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.
    E. Robert Tisdale, Jul 2, 2003
    #17
  18. Ed Avis wrote:
    >>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.


    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


    [ See http://www.gotw.ca/resources/clcm.htm for info about ]
    [ comp.lang.c++.moderated. First time posters: Do this! ]
    Albrecht Fritzsche, Jul 2, 2003
    #18
  19. wired

    John Potter Guest

    On 1 Jul 2003 15:34:11 -0400, Francis Glassborow
    <> wrote:

    > 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&);
    iterator vector<T>::insert(iterator, T 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

    [ See http://www.gotw.ca/resources/clcm.htm for info about ]
    [ comp.lang.c++.moderated. First time posters: Do this! ]
    John Potter, Jul 2, 2003
    #19
  20. In message <>, E. Robert Tisdale
    <> writes
    > > 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.


    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.

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


    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.

    >
    > > 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::eek:stream& foo(std::eek:stream& os){
    > > std::eek:s << std::clock();
    > > return os;
    > > }


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

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

    or even:

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

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


    --
    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! ]
    Francis Glassborow, Jul 2, 2003
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Replies:
    1
    Views:
    271
    Richard Bos
    Apr 10, 2008
  2. Replies:
    0
    Views:
    97
  3. Kedar Mhaswade
    Replies:
    14
    Views:
    296
    Adam Prescott
    Jan 17, 2011
  4. Elegance is an attitude

    , Oct 19, 2008, in forum: Javascript
    Replies:
    0
    Views:
    100
  5. John Ladasky
    Replies:
    10
    Views:
    231
    Peter Cacioppi
    Oct 8, 2013
Loading...

Share This Page