Barnes-Hut implementation of the N-Body problem translated from F# to C#

I am after a peer review of a C# implementation of the Barnes-Hut algorithm which I have translated from F#. The F# version is the base for comparison, therefore the C# version is suppose to reflect the F# in structure and behaviour. I have tried to translate the F# implementation as accurately as possible, however i would like a second opinion.

I would be grateful for any advice e.g. on any problems, or on how to make the algorithm further reflect the F# version. Also, any suggestions on how improve parallel performance, if any.

F# version main algorthim http://pastebin.com/AMb9AnNd

C# version:

```
/*
*
*
* usage:
* args[0] = algorithm version
* args[1] = load balanced
* args[2] = body count
* args[3] = core count
* args[4] = test discription
* args[5] = eps value
*/

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace BHTuned
{
#region BHMain: attributes
///
///
///
public class Body
{
public double x { get; set; }
public double y { get; set; }
public double z { get; set; }
public double vx { get; set; }
public double vy { get; set; }
public double vz { get; set; }
public double m { get; set; }
public Body(double x, double y, double z, double vx, double vy, double vz, double mass)
{
this.x = x;
this.y = y;
this.z = z;
this.vx = vx;
this.vy = vy;
this.vz = vz;
this.m = mass;
}
}

///
///
///
public class Accel
{
public double ax { get; set; }
public double ay { get; set; }
public double az { get; set; }
public Accel(double ax, double ay, double az)
{
this.ax = ax;
this.ay = ay;
this.az = az;
}
}
///

I think there is a lot in the code which can be improved especially from design point of view. A few points which caught my eye follow but it can probably improved further. I would start with what I've written below and then possibly go through another round of refactoring.

Naming conventions.

• C# standard naming convention for methods and public properties is CamelCase.

• Please don't try and save characters on class names. Ideally reading the class name should give you a good indication of what it's purpose. It's not easy to find a good yet concise name but it pays to spend 5min thinking about it. Some suggestions:

• BBox -> BoundingBox

• Accel -> Acceleration

• BHMain -> BarnesHutBase

• Seq -> BarnesHutSequential

• PFor -> BarnesHutParallel

• Please give local variables some more meaningful names. Reading all this bb, ba, b, bl, etc. just hurts the eye. Intellisense and auto completions have been invented a long time ago, no need to try and save characters for sake of typing. Using names like boundingBox, bodies, etc. would make the code much easier to read.

Design

-
Consider making plain data classes like Accel, Centroid etc. structs and immutable. Their properties don't change after they have been constructed if I read your code correctly so you might want to reflect this in your design. Immutability makes a lot of things easier. Eric Lippert has written some very interesting blog articles about immutability. At least consider making the properties which don't change either readonly fields or give them a private set.

-
The findBounds method in BHMain should probably live as a factory method in BBox instead.

-
The calcCentroid method in BHMain should probably live as a factory method in Centroid instead.

-
splitPoints should live in BBox (factory method)

-
buildTree should live in BHTree (factory method)

-
accel should live in Accel (factory method)

-
inbox should live as IsInBox on BBox (non-static)

-
There seems to be a magic for i: 1 -> 16 loop in your doSteps implementations which seems odd.

-
Once you actually remove all those static helper classes from BHMain then there is not much left in BHMain so ask yourself: What should be the purpose of this class?

-
You repeat a lot of code in Seq, PFor and PInvoke - especially the last two share a lot. The central point of the calculations seems to be this block:

Accel acc = calcAccel(BHT, ba[j], eps);
 Body nb = new Body(ba[j].x, ba[j].y, ba[j].z, (ba[j].vx - acc.ax), (ba[j].vy - acc.ay), (ba[j].vz - acc.az), ba[j].m);
 bl[j] = (new Body((nb.x + timeStep * nb.vx), (nb.y + timeStep * nb.vy), (nb.z + timeStep * nb.vz), nb.vx, nb.vy, nb.vz, nb.m));

You should make this a SingleStep method on BHMain which you can call when needed (which gives some purpose back to BHMain)

I would consider a design along these lines:

class BarnesHutBase
 {
     protected void SingleStep(....)
     {
          ...
     }
 }

 interface IStepable
 {
     void PerformSteps(int numberOfSteps);
 }

 class BarnesHutSequential : BarnesHutBase, IStepable
 {
     public void PerformSteps(int numberOfSteps)
     {
         ...
     }
 }

-
The timing and recording of things should be encapsulated in SetFiles as a RecordTime method. Something along these lines:

class RecordingSetFiles : IDisposable
 {
     private Stopwatch _Stopwatch ;
     private SetFiles _SetFiles;
     public RecordingSetFiles(SetFiles files)
     {
         _SetFiles = files;
         _Stopwatch = new Stopwatch();
         _Stopwatch.Start();
     }

     public void Dispose()
     {
         _Stopwatch.Stop();
         _SetFiles.addData(_Stopwatch.ElapsedMilliseconds);
         _SetFiles.buildDataFile();
         _SetFiles.buildRefFile();
     }
 }

 class SetFiles
 {
     public RecordingSetFiles RecordTime()
     {
          return new RecordingSetFiles(this);
     }
 }

Then you can use it like this:

using (sf.RecordTime())
 {
      ....
 }

Possible Bugs

-
In PFor and PInvoke you do this:

List bl = ba; //populate list for iteration

This will not create a new list bl but just assign the reference to bl, so bl and ba now point to the same list. Did you actually meant to do bl = new List(ba)?

StackExchange Code Review Q#35125, answer score: 2