Is this class design correct? A better way?

Discussion in 'C++' started by nw, Feb 16, 2007.

  1. nw

    nw Guest

    Hi,

    I was wondering if someone would be able to give me some comments on
    the following class structure, it feels to me as if there must be a
    better way, but I'm unsure what it is, perhaps I should be using
    multiple inheritance?

    Basically I have a pure virtual class called Body, this contains a
    number of integration algorithms which are applied to the Body.
    Generally only one integration algorithm will be used with a
    particular Body. The integration algorithm is called by the
    integrate() method, which selects the integration algorithm depending
    on how you have set the variable integration_type.

    I'm not sure if it's relevant to this discussion but Body is the base
    class from which two others are derived, these implement the
    calculate_force() method, this method updates a variable used by the
    integrate() method.

    Compilable example code implementing this design follows. Apologies if
    I've been overly verbose.

    Any help greatly appreciated!

    #include <iostream>

    using namespace std;

    class Body {

    public:
    double x, y, z;
    double a;

    static const int BODY_INTEGRATE_EULER=1;
    static const int BODY_INTEGRATE_VERLET=2;
    static const int BODY_INTEGRATE_LEAPFROG=3;

    int integration_type;

    virtual bool calculate_force(Body &b) = 0;

    Body() {
    integration_type=BODY_INTEGRATE_EULER;
    }

    bool integrate() {
    if(integration_type == BODY_INTEGRATE_EULER) {
    // .. do euler
    cout << "Euler integration" << endl;
    return true;
    } else
    if(integration_type == BODY_INTEGRATE_VERLET) {
    // .. do verlet
    cout << "Verlet integration" << endl;
    return true;
    } else
    if(integration_type == BODY_INTEGRATE_LEAPFROG) {
    // .. do leapfrog
    cout << "Leapfrog integration" << endl;
    return true;
    }
    }
    };

    class Planet : public Body {

    virtual bool calculate_force(Body &b) {
    // do force calculation for planet... updates a
    }
    };

    class SimpleHarmonic : public Body {

    virtual bool calculate_force(Body &b) {
    // do force calculation for simple harmonic motion... updates a
    }
    };

    int main(void) {
    Planet p;
    Planet p2;
    SimpleHarmonic s;

    p.integration_type = Body::BODY_INTEGRATE_LEAPFROG;
    p2.integration_type = Body::BODY_INTEGRATE_VERLET;
    s.integration_type = Body::BODY_INTEGRATE_VERLET;

    cout << "planet,leapfrog: ";
    p.integrate();

    cout << "planet,verlet: ";
    p2.integrate();

    cout << "simpleharmonic,verlet: ";
    s.integrate();

    return 0;
    }
     
    nw, Feb 16, 2007
    #1
    1. Advertising

  2. * nw:
    >
    > I was wondering if someone would be able to give me some comments on
    > the following class structure, it feels to me as if there must be a
    > better way, but I'm unsure what it is, perhaps I should be using
    > multiple inheritance?


    You should be using /inheritance/ and-or /templating/ instead of a type
    switch.

    For inheritance, you can represent the integration algorithm directly as
    a derived class override of a Body virtual member function, or you can
    represent it as an object passed to the Body constructor(s).


    [snip]
    > Compilable example code implementing this design follows. Apologies if
    > I've been overly verbose.
    >
    > Any help greatly appreciated!


    OK...


    > #include <iostream>


    Formally you also need <ostream>.


    > using namespace std;
    >
    > class Body {
    >
    > public:
    > double x, y, z;
    > double a;


    Public member variables = ungood.

    Name 'a' = ungood.


    > static const int BODY_INTEGRATE_EULER=1;
    > static const int BODY_INTEGRATE_VERLET=2;
    > static const int BODY_INTEGRATE_LEAPFROG=3;


    All uppercase for non-macros = ungood.


    > int integration_type;


    'int' to represent something with limited number of possible values =
    ungood.

    Representing type as value = in general, and in this case, ungood.


    > virtual bool calculate_force(Body &b) = 0;
    >
    > Body() {
    > integration_type=BODY_INTEGRATE_EULER;
    > }


    See the FAQ item titled 'Should my constructors use "initialization
    lists" or "assignment"?', currently available at <url:
    http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.6>.


    > bool integrate() {
    > if(integration_type == BODY_INTEGRATE_EULER) {
    > // .. do euler
    > cout << "Euler integration" << endl;
    > return true;
    > } else
    > if(integration_type == BODY_INTEGRATE_VERLET) {
    > // .. do verlet
    > cout << "Verlet integration" << endl;
    > return true;
    > } else
    > if(integration_type == BODY_INTEGRATE_LEAPFROG) {
    > // .. do leapfrog
    > cout << "Leapfrog integration" << endl;
    > return true;
    > }
    > }


    This type selection is what you need to avoid.

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
     
    Alf P. Steinbach, Feb 16, 2007
    #2
    1. Advertising

  3. nw

    Piyo Guest

    Hi, I hope this helps in some way. Note: I have no clue what your
    problem domain is but I have some indication that the following
    sample code will help you in some way. BTW, I did NOT try to compile
    the code. I merely wrote it to give you some idea.

    Good Luck!
    PS. I like to use boost. If you don't, RcPtr can be replaced with
    a raw pointer. (www.boost.org)
    -------------------------------------------------------------------

    #include <boost/shared_ptr.hpp>

    class IntegratorImpl
    {
    public:
    typedef boost::shared_ptr<IntegratorImpl> RcPtr;

    public:
    IntegratorImpl( double a ) : m_a( a ) {}

    virtual bool integrate() = 0;

    protected:
    // I think that a should be here not in Body
    // but I don't know what a is.
    double m_a;
    };

    class EulerIntegrator : public IntegratorImpl
    {
    public:
    EulerIntergrator( double a ) : IntegratorImpl( a ) {}

    virtual bool integrate()
    {
    // do Euler integration
    }
    private:
    // put Euler integration specific stuff here
    };

    // add Vertlet and LeapFrog integrators as separate classes like
    // EulerIntegrator

    class Integrator
    {
    public:
    enum IntegrateType
    {
    kNone, kEuler, kVertlet, kLeapFrog
    };
    public:
    Integrator();
    ~Integrator();

    void selectIntegrator( Integrator type, double a )
    {
    switch( type )
    {
    case kEuler:
    m_impl.reset( new EulerIntegrator( a ));
    break;
    case kVertlet:
    m_impl.reset( new VertletIntegrator( a ));
    // you get the idea
    default:
    // throw? your choice
    }
    }

    bool integrate()
    {
    if( m_impl )
    {
    return false;
    }
    else
    {
    return m_impl->integrate();
    }
    }

    private:
    // just a raw pointer if you do not like boost
    IntegratorImpl::RcPtr m_impl;
    };

    class Body
    {
    public:
    Body();

    bool integrate() { return m_integrator.integrate(); }

    virtual void selectIntegrator( Integrator::IntegratorType type )
    {
    // assuming 0 is a good default for a here
    m_integrator.selectIntegrator( type, 0 );
    }

    private:
    Integrator m_integrator;
    // rest of your data members here
    };

    class Planet : public Body
    {
    public:
    Planet( const Body &body ) : m_body( body ) {}

    double calcForce()
    {
    // use m_body to calculate a here
    double a = 1.0;
    return a;
    }

    virtual void selectIntegrator( Integrator::IntegratorType type )
    {
    m_integrator.selectIntegrator( type, calcForce() );
    }

    private:
    const Body &m_body;
    };

    // something similar for SimpleHarmonic

    int
    main(void)
    {
    Planet p;
    Planet p2;
    SimpleHarmonic s;

    p.selectIntegrator( Integrator::kLeapFrog );
    p2.selectIntegrator( Integrator::kVertlet );
    s.selectIntegrator( Integrator::kVertlet );

    cout << "planet,leapfrog: ";
    p.integrate();

    cout << "planet,verlet: ";
    p2.integrate();

    cout << "simpleharmonic,verlet: ";
    s.integrate();

    return 0;
    }


    nw wrote:
    > Hi,
    >
    > I was wondering if someone would be able to give me some comments on
    > the following class structure, it feels to me as if there must be a
    > better way, but I'm unsure what it is, perhaps I should be using
    > multiple inheritance?
    >
    > Basically I have a pure virtual class called Body, this contains a
    > number of integration algorithms which are applied to the Body.
    > Generally only one integration algorithm will be used with a
    > particular Body. The integration algorithm is called by the
    > integrate() method, which selects the integration algorithm depending
    > on how you have set the variable integration_type.
    >
    > I'm not sure if it's relevant to this discussion but Body is the base
    > class from which two others are derived, these implement the
    > calculate_force() method, this method updates a variable used by the
    > integrate() method.
    >
    > Compilable example code implementing this design follows. Apologies if
    > I've been overly verbose.
    >
    > Any help greatly appreciated!
    >
    > #include <iostream>
    >
    > using namespace std;
    >
    > class Body {
    >
    > public:
    > double x, y, z;
    > double a;
    >
    > static const int BODY_INTEGRATE_EULER=1;
    > static const int BODY_INTEGRATE_VERLET=2;
    > static const int BODY_INTEGRATE_LEAPFROG=3;
    >
    > int integration_type;
    >
    > virtual bool calculate_force(Body &b) = 0;
    >
    > Body() {
    > integration_type=BODY_INTEGRATE_EULER;
    > }
    >
    > bool integrate() {
    > if(integration_type == BODY_INTEGRATE_EULER) {
    > // .. do euler
    > cout << "Euler integration" << endl;
    > return true;
    > } else
    > if(integration_type == BODY_INTEGRATE_VERLET) {
    > // .. do verlet
    > cout << "Verlet integration" << endl;
    > return true;
    > } else
    > if(integration_type == BODY_INTEGRATE_LEAPFROG) {
    > // .. do leapfrog
    > cout << "Leapfrog integration" << endl;
    > return true;
    > }
    > }
    > };
    >
    > class Planet : public Body {
    >
    > virtual bool calculate_force(Body &b) {
    > // do force calculation for planet... updates a
    > }
    > };
    >
    > class SimpleHarmonic : public Body {
    >
    > virtual bool calculate_force(Body &b) {
    > // do force calculation for simple harmonic motion... updates a
    > }
    > };
    >
    > int main(void) {
    > Planet p;
    > Planet p2;
    > SimpleHarmonic s;
    >
    > p.integration_type = Body::BODY_INTEGRATE_LEAPFROG;
    > p2.integration_type = Body::BODY_INTEGRATE_VERLET;
    > s.integration_type = Body::BODY_INTEGRATE_VERLET;
    >
    > cout << "planet,leapfrog: ";
    > p.integrate();
    >
    > cout << "planet,verlet: ";
    > p2.integrate();
    >
    > cout << "simpleharmonic,verlet: ";
    > s.integrate();
    >
    > return 0;
    > }
    >
     
    Piyo, Feb 17, 2007
    #3
  4. nw

    nw Guest

    On Feb 17, 12:17 am, Piyo <> wrote:
    > Hi, I hope this helps in some way. Note: I have no clue what your
    > problem domain is but I have some indication that the following
    > sample code will help you in some way. BTW, I did NOT try to compile
    > the code. I merely wrote it to give you some idea.
    >
    > Good Luck!
    > PS. I like to use boost. If you don't, RcPtr can be replaced with
    > a raw pointer. (www.boost.org)
    > -------------------------------------------------------------------
    >
    > #include <boost/shared_ptr.hpp>
    >
    > class IntegratorImpl
    > {
    > public:
    > typedef boost::shared_ptr<IntegratorImpl> RcPtr;
    >
    > public:
    > IntegratorImpl( double a ) : m_a( a ) {}
    >
    > virtual bool integrate() = 0;
    >
    > protected:
    > // I think that a should be here not in Body
    > // but I don't know what a is.
    > double m_a;
    >
    > };
    >
    > class EulerIntegrator : public IntegratorImpl
    > {
    > public:
    > EulerIntergrator( double a ) : IntegratorImpl( a ) {}
    >
    > virtual bool integrate()
    > {
    > // do Euler integration
    > }
    > private:
    > // put Euler integration specific stuff here
    >
    > };
    >
    > // add Vertlet and LeapFrog integrators as separate classes like
    > // EulerIntegrator
    >
    > class Integrator
    > {
    > public:
    > enum IntegrateType
    > {
    > kNone, kEuler, kVertlet, kLeapFrog
    > };
    > public:
    > Integrator();
    > ~Integrator();
    >
    > void selectIntegrator( Integrator type, double a )
    > {
    > switch( type )
    > {
    > case kEuler:
    > m_impl.reset( new EulerIntegrator( a ));
    > break;
    > case kVertlet:
    > m_impl.reset( new VertletIntegrator( a ));
    > // you get the idea
    > default:
    > // throw? your choice
    > }
    > }
    >
    > bool integrate()
    > {
    > if( m_impl )
    > {
    > return false;
    > }
    > else
    > {
    > return m_impl->integrate();
    > }
    > }
    >
    > private:
    > // just a raw pointer if you do not like boost
    > IntegratorImpl::RcPtr m_impl;
    >
    > };
    >
    > class Body
    > {
    > public:
    > Body();
    >
    > bool integrate() { return m_integrator.integrate(); }
    >
    > virtual void selectIntegrator( Integrator::IntegratorType type )
    > {
    > // assuming 0 is a good default for a here
    > m_integrator.selectIntegrator( type, 0 );
    > }
    >
    > private:
    > Integrator m_integrator;
    > // rest of your data members here
    >
    > };
    >
    > class Planet : public Body
    > {
    > public:
    > Planet( const Body &body ) : m_body( body ) {}
    >
    > double calcForce()
    > {
    > // use m_body to calculate a here
    > double a = 1.0;
    > return a;
    > }
    >
    > virtual void selectIntegrator( Integrator::IntegratorType type )
    > {
    > m_integrator.selectIntegrator( type, calcForce() );
    > }
    >
    > private:
    > const Body &m_body;
    >
    > };
    >
    > // something similar for SimpleHarmonic
    >
    > int
    > main(void)
    > {
    > Planet p;
    > Planet p2;
    > SimpleHarmonic s;
    >
    > p.selectIntegrator( Integrator::kLeapFrog );
    > p2.selectIntegrator( Integrator::kVertlet );
    > s.selectIntegrator( Integrator::kVertlet );
    >
    > cout << "planet,leapfrog: ";
    > p.integrate();
    >
    > cout << "planet,verlet: ";
    > p2.integrate();
    >
    > cout << "simpleharmonic,verlet: ";
    > s.integrate();
    >
    > return 0;
    >
    > }
    > nw wrote:
    > > Hi,

    >
    > > I was wondering if someone would be able to give me some comments on
    > > the following class structure, it feels to me as if there must be a
    > > better way, but I'm unsure what it is, perhaps I should be using
    > > multiple inheritance?

    >
    > > Basically I have a pure virtual class called Body, this contains a
    > > number of integration algorithms which are applied to the Body.
    > > Generally only one integration algorithm will be used with a
    > > particular Body. The integration algorithm is called by the
    > > integrate() method, which selects the integration algorithm depending
    > > on how you have set the variable integration_type.

    >
    > > I'm not sure if it's relevant to this discussion but Body is the base
    > > class from which two others are derived, these implement the
    > > calculate_force() method, this method updates a variable used by the
    > > integrate() method.

    >
    > > Compilable example code implementing this design follows. Apologies if
    > > I've been overly verbose.

    >
    > > Any help greatly appreciated!

    >
    > > #include <iostream>

    >
    > > using namespace std;

    >
    > > class Body {

    >
    > > public:
    > > double x, y, z;
    > > double a;

    >
    > > static const int BODY_INTEGRATE_EULER=1;
    > > static const int BODY_INTEGRATE_VERLET=2;
    > > static const int BODY_INTEGRATE_LEAPFROG=3;

    >
    > > int integration_type;

    >
    > > virtual bool calculate_force(Body &b) = 0;

    >
    > > Body() {
    > > integration_type=BODY_INTEGRATE_EULER;
    > > }

    >
    > > bool integrate() {
    > > if(integration_type == BODY_INTEGRATE_EULER) {
    > > // .. do euler
    > > cout << "Euler integration" << endl;
    > > return true;
    > > } else
    > > if(integration_type == BODY_INTEGRATE_VERLET) {
    > > // .. do verlet
    > > cout << "Verlet integration" << endl;
    > > return true;
    > > } else
    > > if(integration_type == BODY_INTEGRATE_LEAPFROG) {
    > > // .. do leapfrog
    > > cout << "Leapfrog integration" << endl;
    > > return true;
    > > }
    > > }
    > > };

    >
    > > class Planet : public Body {

    >
    > > virtual bool calculate_force(Body &b) {
    > > // do force calculation for planet... updates a
    > > }
    > > };

    >
    > > class SimpleHarmonic : public Body {

    >
    > > virtual bool calculate_force(Body &b) {
    > > // do force calculation for simple harmonic motion... updates a
    > > }
    > > };

    >
    > > int main(void) {
    > > Planet p;
    > > Planet p2;
    > > SimpleHarmonic s;

    >
    > > p.integration_type = Body::BODY_INTEGRATE_LEAPFROG;
    > > p2.integration_type = Body::BODY_INTEGRATE_VERLET;
    > > s.integration_type = Body::BODY_INTEGRATE_VERLET;

    >
    > > cout << "planet,leapfrog: ";
    > > p.integrate();

    >
    > > cout << "planet,verlet: ";
    > > p2.integrate();

    >
    > > cout << "simpleharmonic,verlet: ";
    > > s.integrate();

    >
    > > return 0;
    > > }


    Thank you for both your and Alf P. Steinbach's suggestions.

    In my example, a is acceleration. It is a quantity calculated from
    other member variables of Body (not shown) and used by unshown members
    of Body. I suppose I could pass the integrator object a reference to
    Body through which it could access these values, but then I guess I
    have lots of integrator objects which only contain a reference to
    Body? I think this would basically mean changing the Integrators to
    something like:

    class EulerIntegrator : public IntegratorImpl
    {
    EulerIntegrator(Body &b) : IntegratorImpl(b) {}

    bool integrate()
    {
    Vec velocity = m_body.get_velocity();
    Vec acceleration = m_body.get_acceleration();
    Ensemble &m_ensemble = m_body.get_ensemble();

    velocity += acceleration * m_ensemble.get_dt();
    position += velocity * m_ensemble.get_dt();
    }
    }

    It seems from this I would have a lot of largely empty integrator
    objects, one for each body. Is it possible to avoid this? Are there
    other alternatives I should consider?

    Thanks again for your help.
     
    nw, Feb 21, 2007
    #4
  5. nw

    Jerry Coffin Guest

    In article <>,
    says...

    [ ... ]

    > In my example, a is acceleration. It is a quantity calculated from
    > other member variables of Body (not shown) and used by unshown members
    > of Body. I suppose I could pass the integrator object a reference to
    > Body through which it could access these values, but then I guess I
    > have lots of integrator objects which only contain a reference to
    > Body? I think this would basically mean changing the Integrators to
    > something like:
    >
    > class EulerIntegrator : public IntegratorImpl
    > {
    > EulerIntegrator(Body &b) : IntegratorImpl(b) {}
    >
    > bool integrate()
    > {
    > Vec velocity = m_body.get_velocity();
    > Vec acceleration = m_body.get_acceleration();
    > Ensemble &m_ensemble = m_body.get_ensemble();
    >
    > velocity += acceleration * m_ensemble.get_dt();
    > position += velocity * m_ensemble.get_dt();
    > }
    > }


    At least to me, the integrators look like good candidates to become
    functors to be applied to Bodies:

    struct Euler_Integrator {
    void operator()(Body &b) {
    double dt = b.get_ensemble().get_dt();
    Vec velocity = b.get_velocity() +
    b.get_acceleration() * dt();
    TYPE position = velocity * dt();
    // probably need to do something with
    // velocity and/or position here...
    }
    };

    // likewise for Verlet and Leapfrog integrators.

    int main() {
    Planet p;
    Planet p2;

    Euler_Integrator e;
    Verlet_Integrator v;

    e(p);
    v(p2);

    return 0;
    }

    > It seems from this I would have a lot of largely empty integrator
    > objects, one for each body. Is it possible to avoid this? Are there
    > other alternatives I should consider?


    I'm not sure what makes you think there would be largely empty
    integrators for each body, so it's hard to guess how to avoid them. I
    seem to have missed at least part of the thread, but I'm not entirely
    clear on the significance of 'a' either -- and/or whether it's replaced
    and/or affected by get_acceleration in the code above.

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
     
    Jerry Coffin, Feb 24, 2007
    #5
    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. joon
    Replies:
    1
    Views:
    515
    Roedy Green
    Jul 8, 2003
  2. Dan

    correct or not correct?

    Dan, Oct 2, 2003, in forum: HTML
    Replies:
    7
    Views:
    445
  3. J.Ram
    Replies:
    7
    Views:
    654
  4. Paul Rubin
    Replies:
    5
    Views:
    417
    Hendrik van Rooyen
    Aug 6, 2009
  5. Replies:
    2
    Views:
    56
    Mark H Harris
    May 13, 2014
Loading...

Share This Page