2
\$\begingroup\$

My code is meant to update the positions and velocities of planets. There are a few methods in this class, but updatePosition and updateVelocity are the most important, as they implement the velocity verlet algorithm. Could it be shorter/more efficient?

public class Particle{

private double mass;
private PhysicsVector initialPosition = new PhysicsVector();
private PhysicsVector initialVelocity = new PhysicsVector();

public PhysicsVector updatePosition(PhysicsVector initialPosition, PhysicsVector initialVelocity, double timeStep, PhysicsVector aDueToGravity){
    PhysicsVector x = new PhysicsVector(initialVelocity);
    x.scale(timeStep);
    PhysicsVector z = new PhysicsVector(aDueToGravity);
    z.scale(0.5*timeStep*timeStep);

    initialPosition.increaseBy(x);
    initialPosition.increaseBy(z);
    return initialPosition;
}

public PhysicsVector updateVelocity(PhysicsVector initialVelocity, double timeStep, PhysicsVector a, PhysicsVector newA){
    PhysicsVector z = new PhysicsVector(newA);
    z.increaseBy(a);
    z.scale(0.5*timeStep);
    initialVelocity.increaseBy(z);
    return initialVelocity;
}

public PhysicsVector[] centreOfMass(double[] mass, PhysicsVector[] positions){
    PhysicsVector centreOfMass = new PhysicsVector();   
    centreOfMass.setVector(0,0);
    double sum = ArrayLogic.sumArray(mass);
    for(int i=0;i<positions.length;i++){
        centreOfMass.increaseBy(positions[i].scale(mass[i],positions[i]));
    }
    centreOfMass.scale(1/sum);
    positions = ArrayLogic.subtract(positions, centreOfMass);
    return positions;
}

public PhysicsVector[] cOMVel(double[] mass, PhysicsVector[] velocity){
    PhysicsVector cOMV = new PhysicsVector();
    double total = ArrayLogic.sumArray(mass);
    for(int ae=0;ae<velocity.length;ae++){
        cOMV.increaseBy(velocity[ae].scale(mass[ae],velocity[ae]));
    }
    cOMV.scale(1/total);
    velocity = ArrayLogic.subtract(velocity, cOMV); 
    return velocity;
}

public static PhysicsVector[] momentum(double[] planetMass, PhysicsVector[] velocity, PhysicsVector[] positions){
    PhysicsVector[] momentum = new PhysicsVector[planetMass.length];
    PhysicsVector[] angularMomentum = new PhysicsVector[planetMass.length];
    PhysicsVector a = new PhysicsVector(0,0);
    for(int i=0; i<velocity.length; i++){
        a = a.scale(planetMass[i], velocity[i]);
        momentum[i] = a;
    }

    for(int i=0; i<velocity.length; i++){
        angularMomentum[i] = angularMomentum[i].vectorProduct(positions[i], momentum[i]);
    }

    return angularMomentum;
   }

}

At the moment, increasing the timestep seems to improve my results. This seems odd, which is partly why I suspect my implementation is incorrect.

\$\endgroup\$
  • 1
    \$\begingroup\$ May help to have code for PhysicsVector in order to see scale, increaseBy method code. Also of help would be some known input, expected debug output or values. \$\endgroup\$ – kph0x1 Jan 5 '16 at 19:19
2
\$\begingroup\$

In writing a class model, it can be useful to consider whether a class represents a value (something that doesn't change, but can be useful in multiple contexts), or an entity (something with an identity - a sense of self - that doesn't change even though the values associated with that identity do change).

In your fragment here, I would expect Particle to be an entity (Earth is still Earth even while it moves through its orbit), and PhysicsVector to be a value.

What does this mean for your implementations?

Entities are usually an encapsulation of some rules describing how their current state becomes the next state. You don't change the state of an entity directly - instead you command it to change, giving it the arguments that it needs to do the work, and the entity determines its own next state.

public class Particle{

    private double mass;
    private PhysicsVector initialPosition = new PhysicsVector();
    private PhysicsVector initialVelocity = new PhysicsVector();

Quick review here - these describe the state of the particle, and you've made them private, which is consistent with the idea that the Particle should control its own state. mass should probably be final -- you could name it restMass to be more clear. (It might not be final, depending on how you want to model inelastic collisions).

initialPosition and initialVelocity are probably incorrectly named -- the "initial" state of an object shouldn't change later? so currentPosition/currentVelocity would make things clearer, although since the entity is always holding its current state, position and velocity would probably suffice.

My guess is that your model will want to be able to specify the starting configuration of particles, so you should have a constructor that allows them to do so -- in other words, we are constructing the entity in its initial state.

public class Particle{
    // ...
    public Particle(double restMass, PhysicsVector initialPosition, PhysicsVector initialVelocity) {
        this.mass = restMass;
        this.position = initialPosition;
        this.velocity = initialVelocity;

One constructor can reference another, so if you intend to support the creation of particles in an arbitrary initial state, you create a second contructor that calls this first:

    public Particle(double restMass) {
        this(restMass, new PhysicsVector(), new PhysicsVector());

Now, as we said we send commands to an entity to tell it to change its state. The entity already knows its own state, so we don't need to pass that in. So this code doesn't really make sense

public PhysicsVector updatePosition(PhysicsVector initialPosition, PhysicsVector initialVelocity, double timeStep, PhysicsVector aDueToGravity){
    PhysicsVector x = new PhysicsVector(initialVelocity);
    x.scale(timeStep);
    PhysicsVector z = new PhysicsVector(aDueToGravity);
    z.scale(0.5*timeStep*timeStep);

    initialPosition.increaseBy(x);
    initialPosition.increaseBy(z);

    return initialPosition;
}

Either this is a member function (meaning, being run against a specific entity), in which case it should be using the state of the entity at the time it is run:

public void updatePosition(double timeStep, PhysicsVector aDueToGravity){
    PhysicsVector x = new PhysicsVector(velocity);
    x.scale(timeStep);
    PhysicsVector z = new PhysicsVector(aDueToGravity);
    z.scale(0.5*timeStep*timeStep);

    position.increaseBy(x);
    position.increaseBy(z);

    return initialPosition;
}

or instead it's more a convenient calculation service; you can make it clear that it isn't really scoped to the entity by declaring the method to be static (class scope)

public static PhysicsVector updatePosition(PhysicsVector initialPosition, PhysicsVector initialVelocity, double timeStep, PhysicsVector aDueToGravity){
    // ...
}

Or alternatively you can perhaps see in it a time step calculator that is independent of the Particle

public class Accelerator {
    private final double timeStep;
    private final PhysicsVector acceleration;

    public PhysicsVector nextPosition(PhysicsVector currentPosition, PhysicsVector currentVelocity) {
        // ...
    }

public class Particle {
    public void updatePosition(Accelerator accelerator) {
        position = accelerator.nextPosition(position, velocity);

I strongly prefer this approach -- gravity, in non uniform fields, depends on where you are, so you are going to need the position to calculate the force (and then the mass to scale it to the right acceleration).

Your calculator for momentum definitely looks more like a service that should be operating on a collection of particles, than it does a member function of a single particle

public static PhysicsVector[] momentum(Particle [] particles)

ValueTypes are typically immutable - subtracting the position of the earth from the position of the sun doesn't move either particle, but instead gives you a new vector describing the relative position between the two.

We don't send commands to valueTypes, we send them queries, "what do we get if?" kinds of things. Running the query doesn't change the value (it is immutable), it just returns a result. So if we want to use vector addition to update the position of the particle, we should expect to use a replace with query result pattern

position = position.increaseBy(x);

Given that what we are doing is vector addition, I'd probably prefer that increaseBy was instead using the spelling add.

Other modeling considerations - PhysicsVector, as a type, is a bit vague; it would be very easy to initialize a particle the wrong way round by ordering your position and velocity incorrectly. You could make PositionVector distinct from VelocityVector; this would also guard against bugs where you try to add two different vectors that have different units. Depending on the model, you might also want to distinguish PolarVectors and AxialVectors,

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow! I appreciate now how vague my question was, and how incomplete. PhysicsVector is its own class that deals with all sorts of operations on vectors, and Particle is an entity. I do also have a class that deals with gravity. Thank you for a very useful answer! Especially given so little information to work with. \$\endgroup\$ – user13948 Jan 6 '16 at 12:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.