6
\$\begingroup\$

I'm working on some precompilation operations for a world compiler. Currently to identify flags placed by the level designer I need recognize when a specific entity exists at specific coordinates from the flag's origin. I need to generate a list of offset coordinates from the origin(and include the origin) in string form. That is 7 coordinates. this operation is done for every flag in the world, so it is preferred to be efficient. It is not an operation that takes place during gameplay, so I don't have to worry too much.

My current implementation works correctly, and quickly, however I still would like to get a review on what I have written, and hear any advice to better this operation.

string origin = "0 0 0";
double[] originParts = origin.Split(' ').Select(s => double.Parse(s)).ToArray();
List<string> origins = new List<string> { origin };
for (int i = 0; i < 3; i++)
    for (int j = -48; j <= 48; j += 96)
        origins.Add(string.Format("{0} {1} {2}", 
            originParts[0] + (i == 0 ? j : 0),
            originParts[1] + (i == 1 ? j : 0), 
            originParts[2] + (i == 2 ? j : 0)));
\$\endgroup\$
3
\$\begingroup\$

This code is effectively calculating a set of offsets from the origin and adding them in turn to get the final co-ordinates. However, you're calculating the offsets each time for every flag and the question implies that this is done many times. Better to calculate them up-front, then iterate through them, applying the co-ordinates of the flag to each:

// Done once at initialization...
int[,] offsets = new int[6, 3] { { -48,   0,   0 }, 
                                 {  48,   0,   0 }, 
                                 {   0, -48,   0 }, 
                                 {   0,  48,   0 }, 
                                 {   0,   0, -48 },
                                 {   0,   0,  48 } }; ;

// Done for each flag in the game world...
string origin = "0 0 0";
double[] originParts = origin.Split(' ').Select(s => double.Parse(s)).ToArray();
List<string> origins = new List<string> { origin };

for(int idx = 0; idx < offsets.GetLength(0); idx++)
{
    origins.Add(String.Format("{0} {1} {2}", 
        originParts[0] + offsets[idx, 0],
        originParts[1] + offsets[idx, 1],
        originParts[2] + offsets[idx, 2]));
}

You could, of course, build the array of offsets programmatically instead of hardcoding them like I did.

This gives you slightly more lines of code, but it reduces the number of calculations done on each loop, and separates the calculation of the offsets from the calculation of the final co-ordinate, which may make future modifications simpler (e.g. if the designer wants more co-ordinates added to the list, or co-ordinates should be read from a configuration file, and so on).

\$\endgroup\$
  • \$\begingroup\$ This way may be more clear, but I think it has to do about the same amount of work. \$\endgroup\$ – BenVlodgi May 27 '14 at 16:59
3
\$\begingroup\$

Another option is to store the coordinates as a custom class, with the coordinates as doubles and the ToString method overridden to display them in a formatted string.

public class MapPoint
{
    public double X = 0;
    public double Y = 0;
    public double Z = 0;
    //This constructor is in case you need it.
    public MapPoint(string coordinates)
    {
        double[] temp = coordinates.Split(' ').Select(x => double.Parse(x)).ToArray();
        if(temp.Length == 0)
            throw new Exception("Invalid string");
        else
            X = temp[0];
        if(temp.Length > 1)
            Y = temp[1];
        if(temp.Length > 2)
            Z = temp[2];
    }
    public MapPoint(double x, double y = 0, double z = 0)
    {
        X = x;
        Y = y;
        Z = z;
    }
    public MapPoint(MapPoint from)
    {
        X = from.X;
        Y = from.Y;
        Z = from.Z;
    }
    public MapPoint()
    {
    }
    public MapPoint Offset(double x, double y, double z)
    {
        this.X += x;
        this.Y += y;
        this.Z += z;
        return new MapPoint(this);
    }
    public MapPoint Offset(MapPoint offset)
    {
        X += offset.X;
        Y += offset.Y;
        Z += offset.Z;
        return new MapPoint(this);
    }
    public override string ToString()
    {
        return String.Format("{0} {1} {2}", X, Y, Z);
    }
}

In your Flag class it doesn't become unwieldy to create the list of coordinates without a loop, since there are only seven:

public class Flag
{
    public string Name = "";
    public MapPoint origin = new MapPoint();
    List<MapPoint> origins;
    public Flag()
    {
        InitList();
    }
    public Flag(MapPoint neworigin)
    {
        origin = new MapPoint(neworigin);
        InitList();
    }
    private void InitList()
    {
        origins = new List<MapPoint>
        {
            origin.Offset(0,0,0),
            origin.Offset(-48,0,0),
            origin.Offset(48,0,0),
            origin.Offset(0,-48,0),
            origin.Offset(0,48,0),
            origin.Offset(0,0,-48),
            origin.Offset(0,0,48)
        };
    }
}

This assumes your offsets can be hardcoded.

Now every new flag, you pass the origin and the list of offsets is automatically created.

\$\endgroup\$
  • \$\begingroup\$ Just a small nitpick, you are creating public members instead of public Properties. public string Name = ""; instead of public string Name {get; set;} I do like your implementation though. \$\endgroup\$ – BenVlodgi May 27 '14 at 16: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.