Class design (again)

[nw]

Hi,

I previously posted a question asking for suggestions on my class
design. My original code consisted of a pure virtual base class Body
which contained a number of integration algorithms which are applied
to the Body. Generally only one integration algorithm is used via an
integrate() method, which selected the integration algorithm to call
depending on a variable.

Code similar to the following was suggested as a improvement. I have
modified it so it should compile on gcc. In the posters example
acceleration had also been moved from Body to Integrator, in the wider
context of my code I don't think I can do this. Here is the new class
design:

#include <iostream>
#include <ostream>

using namespace std;

class Body;
class IntegratorImpl;

class Integrator {
public:

Integrator() {}
~Integrator() {}

enum IntegratorType
{
kNone, kEuler, kVerlet, kLeapFrog
};

bool integrate();
void selectIntegrator(IntegratorType t, Body &b);

private:
IntegratorImpl *m_impl;
};

class Body
{
public:
Body() {}

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

void selectIntegrator( Integrator::IntegratorType type )
{
m_integrator.selectIntegrator( type, *this );
}

Integrator m_integrator;
double position_x, position_y, position_z;
double velocity_x, velocity_y, velocity_z;
double acceleration_x, acceleration_y, acceleration_z;
static const double dt = 1;
};

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

virtual bool integrate() = 0;

protected:
Body *m_body;
};


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

bool integrate() {
// do Euler integration
m_body->velocity_x += m_body->acceleration_x * m_body->dt;
m_body->position_x += m_body->velocity_x * m_body->dt;
// etc...
cout << "Euler integration called" << endl;
return true;
}
};

class VerletIntegrator : public IntegratorImpl {
public:
VerletIntegrator(Body &m_body) : IntegratorImpl(m_body) {}

bool integrate() {
// do Verlet integration
// etc...
cout << "Verlet integration called" << endl;
return true;
}
};

// Class LeapfrogIntegrator similar to EulerIntegrator not shown

void Integrator::selectIntegrator(IntegratorType type,Body &body) {
switch( type ) {
case kEuler:
m_impl = new EulerIntegrator(body);
break;
case kVerlet:
m_impl = new VerletIntegrator(body);
break;
default:
// throw? your choice
break;
}
}
bool Integrator::integrate() {
return m_impl->integrate();
}

class Planet : public Body {
public:
Planet() {}

double calcForce(Planet &p)
{
acceleration_x = (p.position_x - position_x) * (p.position_x -
position_x);
}
};

// Class SimpleHarmonic, not shown defined similar to Planet

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

p.selectIntegrator( Integrator::kEuler );
p2.selectIntegrator( Integrator::kVerlet );
// s.selectIntegrator( Integrator::kLeapfrog );

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

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

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

return 0;
}

In this code an Integrator object is stored by Body, Integrator is
provided with a reference to Body which it uses to access the data
from Body it needs to perform the integration (this should probably be
encapsulated better than it is in the example code) and a value
specifying which type of integrator to use. The Integrator object
creates an IntergratorImpl object depending on this type. And when
Integrator.integrate() is called, the call is passed on to the
underlying implementation.

I believe this is an improvement over my original code but that this
solution would construct lots of largely empty integrator objects,
which only contain a pointer to Body. Is it possible to avoid this?
And also perhaps the overhead caused by the Integrator object? Are
there other solutions I should consider?

The previous posters, and any additional advice is appreciated.

The original thread is here:

http://groups.google.ie/group/comp.lang.c++/browse_thread/thread/529aabfa5c5b8f9b/5503c8ebcd5cbe9d

 

[Piyo]

I believe this is an improvement over my original code but that this
solution would construct lots of largely empty integrator objects,
which only contain a pointer to Body. Is it possible to avoid this?
And also perhaps the overhead caused by the Integrator object? Are
there other solutions I should consider?

It's good to see you've adapted my original suggestion to you. What
you have just implemented is a Strategy Pattern. It is generally a
good idea to keep it the way you implemented. As for instancing largely
empty objects, these empty objects by design help you mix and match
different integration methods without impacting the API that much. This
means, creating a different integration method will not propagate
a lot of code change outside the Strategy pattern. Also, as you have
noticed, your old switch statement is replaced by the virtual integrate
calls thus keeping your code cleaner. So not unless there is a need
to keep memory to a very minimum, I highly recommend to keep it that
way since you are trading off future extensibility and maintainability
of your code if you remove it. The run-time performance should also not
be that big of an issue since the majority of your time should be spent
on integrating anyway so the overhead of a pointer indirection and
a virtual call should be tiny.

If you insist on redesigning it, you can first remove Integrator and
use IntegratorImpl directly inside your Planet. This removes a memory
overhead. You can also then also remove IntegratorImpl and make each
Integration object a Functor (this removes the virtual call overhead).
Then you can use boost::function to make your signatures cleaner and
then you can pass in the functor object at run-time.
(example code at the bottom)

Good Luck with your project!

PS. please please please consider getting boost. I noticed you chose
not to use it. It makes life easier.

Note: this is not complete compilable code. For illustration
purposes only.
---------------------------------------------------------------
#include <boost/function.hpp>

class EulerIntegrator
{
public:
// do what you need to construct it properly
EulerIntegrator();
bool operator()( Body * b )
{
// do Euler Integration
}
};

// same for other integrators

class Planet : public Body // you know how to define Body already
{
public:
// instead of selecting an integrator with a token, you can
// pass a functor to the integrator object directly.
bool integrate( boost::function<bool (Body*)> f )
{
// I am assuming that the body you want to integrate
// with is itself.
return f( this );
}
};


int
main(void)
{
Planet p;
p.integrate( EulerIntegrator() );
}

 

[Michael]

Code similar to the following was suggested as a improvement. I have

modified it so it should compile on gcc. In the posters example
acceleration had also been moved from Body to Integrator, in the wider
context of my code I don't think I can do this. Here is the new class
design:

class Integrator {
public:
bool integrate();
void selectIntegrator(IntegratorType t, Body &b);

private:
IntegratorImpl *m_impl;

};

class Body
{
public:
bool integrate() { return m_integrator.integrate(); }

void selectIntegrator( Integrator::IntegratorType type )
{
m_integrator.selectIntegrator( type, *this );
}
};

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

virtual bool integrate() = 0;

protected:
Body *m_body;

};

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

bool integrate() {
// do Euler integration
m_body->velocity_x += m_body->acceleration_x * m_body->dt;
m_body->position_x += m_body->velocity_x * m_body->dt;
// etc...
cout << "Euler integration called" << endl;
return true;
}

};

I believe this is an improvement over my original code but that this
solution would construct lots of largely empty integrator objects,
which only contain a pointer to Body. Is it possible to avoid this?
And also perhaps the overhead caused by the Integrator object? Are
there other solutions I should consider?

I would suggest you change this:
IntegratorImpl(Body &b) : m_body( &b ) {}
virtual bool integrate() = 0;

To
IntegratorImpl() {}
virtual bool integrate(Body& b) = 0;

Right now, your IntegratorImpl has 2 responsibilities:
1) Store Body
2) Implement integration strategy

My suggestion is to get rid of responsibility 1. (OO guideline -
classes should have exactly 1 responsibility. Not always 100% true,
but a good guideline.)

Once you have done that, then you don't need to allocate a new
strategy object each time you set the integration strategy - instead
you can implement your various strategies as Singletons, and just
store a pointer to each in the selectIntegrator method (note: you
would not own that pointer, so you shouldn't delete it). That way, if
you have 5 strategies and 1000 bodies, you would only have 5 strategy
objects, not 1000.

Michael