Is this class design correct? A better way?

[nw]

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

 

[Alf P. Steinbach]

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

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.

 

[Piyo]

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]

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;

}

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.

 

[Jerry Coffin]

[ ... ]

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.