Mfcttrf

I have been working on a vector library for a little bit now. And at this point the basics are down and I am looking for some tips and advic - this may include:

1. suggestions for things that might be missing and should be added


2. improvements for readability and of-course efficiency


3. if you have an important tip or advice that I didn't think of, please feel free to add it :)

See the code below for a Vector3 struct and a VectorX struct. I have left out the Vector2, 4 for now because those are build in the exact same way as Vector3. VectorX is a struct for vectors which have no predefined length. See code below:

Vector3:

using System;

namespace Development

 public struct Vector3 : IEquatable<Vector3>
 
 public float X get; set; 
 public float Y get; set; 
 public float Z get; set; 
 public int VectorCount get return 3; 

 public Vector3(float value)
 
 X = value;
 Y = value;
 Z = value;
 

 public Vector3(float x, float y, float z)
 
 X = x;
 Y = y;
 Z = z;
 

 public Vector3(Vector3 vector)
 
 X = vector.X;
 Y = vector.Y;
 Z = vector.Z;
 

 public Vector3(float[] arr)
 
 X = arr[0];
 Y = arr[1];
 Z = arr[2];
 

 public Vector3(float[] arr, int startIndex)
 
 if (startIndex > arr.Length - 3) throw new Exception("start index doesnt leave enough space to fill all vector parameters"); 
 else
 
 X = arr[startIndex];
 Y = arr[startIndex + 1];
 Z = arr[startIndex + 2];
 
 


 public override string ToString()
 
 return "<" + X + ", " + Y + ", " + Z + ">";
 

 public float[] ToArray()
 
 return new float[] X, Y, Z ;
 

 public Vector3 UpdateFromArray(float[] arr)
 
 if (arr.Length < 3) throw new Exception("Array is too small to convert to vector");
 else
 
 X = arr[0];
 Y = arr[1];
 Z = arr[2];
 
 return this;
 

 public Vector3 UpdateFromArray(float[] arr, int startIndex)
 
 if (startIndex + 2 > arr.Length - 1) throw new Exception("startindex is too high to fill vector");
 else
 
 X = arr[startIndex];
 Y = arr[startIndex + 1];
 Z = arr[startIndex + 2];
 
 return this;
 

 public bool Equals(Vector3 other) // not implemented
 
 throw new NotImplementedException();
 

 public Vector3 Abs()
 
 X = (X < 0) ? X *= -1 : X;
 Y = (Y < 0) ? Y *= -1 : Y;
 Z = (Z < 0) ? Z *= -1 : Z;
 return this;
 

 public float Length()
 
 return MathF.Sqrt(MathF.Pow(X, 2) + MathF.Pow(Y, 2) + MathF.Pow(Z, 2));
 

 public float Dist(Vector3 other)
 
 return MathF.Sqrt(MathF.Pow((this.X - other.X), 2) + MathF.Pow((this.Y - other.Y), 2) + MathF.Pow((this.Z - other.Z), 2));
 

 public Vector3 Min(Vector3 min)
 
 this.X = (this.X < min.X) ? this.X : min.X;
 this.Y = (this.Y < min.Y) ? this.Y : min.Y;
 this.Z = (this.Z < min.Z) ? this.Z : min.Z;
 return this;
 

 public Vector3 Max(Vector3 max)
 
 this.X = (this.X > max.X) ? this.X : max.X;
 this.Y = (this.Y > max.Y) ? this.Y : max.Y;
 this.Z = (this.Z > max.Z) ? this.Z : max.Z;
 return this;
 

 public Vector3 Clamp(Vector3 min, Vector3 max)
 
 this.X = (this.X < min.X) ? min.X : this.X = (this.X > max.X) ? max.X : this.X;
 this.Y = (this.Y < min.Y) ? min.Y : this.Y = (this.Y > max.Y) ? max.Y : this.Y;
 this.Z = (this.Z < min.Z) ? min.Z : this.Z = (this.Z > max.Z) ? max.Z : this.Z;
 return this;
 

 public Vector3 Lerp(Vector3 to, float amount)
 
 amount = Math.Clamp(amount, 0, 1);
 this.X = (this.X - to.X) * amount + to.X;
 this.Y = (this.Y - to.Y) * amount + to.Y;
 this.Z = (this.Z - to.Z) * amount + to.Z;
 return this;
 

 public Vector3 Normalize()
 
 float scalar = 1 / MathF.Sqrt(MathF.Pow(X, 2) + MathF.Pow(Y, 2) + MathF.Pow(Z, 2));
 X *= scalar;
 Y *= scalar;
 Z *= scalar;
 return this;
 

 public float Dot(Vector3 other)
 
 return this.X * other.X + this.Y * other.Y + this.Z * other.Z;
 


 public static Vector3 Abs(Vector3 value)
 
 return value.Abs();
 

 public static Vector3 Clamp(Vector3 value, Vector3 min, Vector3 max)
 
 return value.Clamp(min, max);
 

 public static Vector3 Min(Vector3 value, Vector3 min)
 
 return value.Min(min);
 

 public static Vector3 Max(Vector3 value, Vector3 max)
 
 return value.Max(max);
 

 public static float Dist(Vector3 value1, Vector3 value2)
 
 return value1.Dist(value2);
 

 public static float Dot(Vector3 value1, Vector3 value2)
 
 return value1.Dot(value2);
 

 public static float Length(Vector3 value)
 
 return value.Length();
 

 public static Vector3 Lerp(Vector3 from, Vector3 to, float amount)
 
 return from.Lerp(to, amount);
 

 public static Vector3 Normalize(Vector3 value)
 
 return value.Normalize();
 


 public static Vector3 Zero get return new Vector3(0); 
 public static Vector3 Unit get return new Vector3(1); 
 public static Vector3 UnitX get return new Vector3(1, 0, 0); 
 public static Vector3 UnitY get return new Vector3(0, 1, 0); 
 public static Vector3 UnitZ get return new Vector3(0, 0, 1); 


 public static Vector3 operator +(Vector3 left, float value)
 
 left.X += value;
 left.Y += value;
 left.Z += value;
 return left;
 

 public static Vector3 operator +(Vector3 left, Vector3 right)
 
 left.X += right.X;
 left.Y += right.Y;
 left.Z += right.Z;
 return left;
 

 public static Vector3 operator -(Vector3 left)
 
 left.X = (-left.X);
 left.Y = (-left.Y);
 left.Z = (-left.Z);
 return left;
 

 public static Vector3 operator -(Vector3 left, float value)
 
 left.X -= value;
 left.Y -= value;
 left.Z -= value;
 return left;
 

 public static Vector3 operator -(Vector3 left, Vector3 right)
 
 left.X -= right.X;
 left.Y -= right.Y;
 left.Z -= right.Z;
 return left;
 

 public static Vector3 operator /(Vector3 left, float value)
 
 left.X /= value;
 left.Y /= value;
 left.Z /= value;
 return left;
 

 public static Vector3 operator /(Vector3 left, Vector3 right)
 
 left.X /= right.X;
 left.Y /= right.Y;
 left.Z /= right.Z;
 return left;
 

 public static Vector3 operator *(Vector3 left, float value)
 
 left.X *= value;
 left.Y *= value;
 left.Z *= value;
 return left;
 

 public static Vector3 operator *(Vector3 left, Vector3 right)
 
 left.X *= right.X;
 left.Y *= right.Y;
 left.Z *= right.Z;
 return left;
 

 // do these size comparisions make sense?
 public static bool operator <(Vector3 left, Vector3 right)
 
 return (left.Length() < right.Length()) ? true : false;
 

 public static bool operator >(Vector3 left, Vector3 right)
 
 return (left.Length() > right.Length()) ? true : false;
 

 public static bool operator >=(Vector3 left, Vector3 right)
 
 return (left.Length() >= right.Length()) ? true : false;
 

 public static bool operator <=(Vector3 left, Vector3 right)
 
 return (left.Length() <= right.Length()) ? true : false;
 
 // end question

 public static bool operator ==(Vector3 left, Vector3 right)
 
 return (left.X == right.X && left.Y == right.Y && left.Z == right.Z) ? true : false;
 

 public static bool operator !=(Vector3 left, Vector3 right)
 left.Z != right.Z) ? true : false;
 

 

VectorX:

using System;

namespace Development

 public struct VectorX : IEquatable<VectorX>
 
 // private float[] VectorValues;
 public float[] VectorValues get; set; 
 public int VectorCount get; private set; 

 public VectorX(int vectorCount)
 
 VectorValues = new float[vectorCount];
 VectorCount = vectorCount;
 

 public VectorX(int vectorCount, float defaultValue)
 
 VectorCount = vectorCount;
 VectorValues = new float[vectorCount];
 for (int i = 0; i < vectorCount; i++)
 
 VectorValues[i] = defaultValue;
 

 

 public VectorX(params float[] values)
 
 VectorValues = values;
 VectorCount = values.Length;
 

 public VectorX(float[] arr, int startIndex)
 
 VectorCount = arr.Length - 1 - startIndex;
 VectorValues = new float[VectorCount];
 for (int i = startIndex; i < arr.Length; i++)
 
 VectorValues[i - startIndex] = arr[i];
 
 

 public VectorX(float[] arr, int startIndex, int endIndex)
 
 VectorCount = endIndex - startIndex;
 VectorValues = new float[VectorCount];
 for (int i = startIndex; i < endIndex; i++)
 
 VectorValues[i - startIndex] = arr[i];
 
 

 public VectorX(VectorX vector)
 
 VectorCount = vector.VectorCount;
 VectorValues = vector.VectorValues;
 


 public override string ToString()
 
 string ret = "<";
 for (int i = 0; i < VectorCount; i++)
 
 if (i == VectorCount - 1) ret += VectorValues[i];
 else ret += VectorValues[i] + ", ";
 
 return ret += ">";
 

 public float[] ToArray()
 
 return VectorValues;
 

 public VectorX UpdateFromArray(float[] arr)
 
 if (arr.Length < VectorCount) throw new Exception("Array is too small to convert to vector");
 else
 
 for (int i = 0; i < VectorCount; i++)
 
 VectorValues[i] = arr[i];
 
 
 return this;
 

 public VectorX UpdateFromArray(float[] arr, int startIndex)
 
 if (startIndex + VectorCount > arr.Length - 1) throw new Exception("startindex is too high to fill vector");
 else
 
 for (int i = 0; i < VectorCount; i++)
 
 VectorValues[i] = arr[startIndex + i];
 
 

 return this;
 

 public float GetAt(int pos)
 
 if (pos >= VectorCount 

 public VectorX SetAt(int pos, float value)
 pos < 0) throw new Exception("Supplied position is too large or less then 0");
 else
 
 VectorValues[pos - 1] = value;
 return this;
 
 

 public bool Equals(VectorX other) // not implemented
 
 throw new NotImplementedException();
 

 public float Length()
 
 float total = 0;
 for (int i = 0; i < VectorCount; i++)
 
 total += MathF.Pow(VectorValues[i], 2);
 
 return MathF.Sqrt(total);
 

 public VectorX Normalize()
 
 float scalar = 1 / this.Length();
 for (int i = 0; i < VectorCount; i++)
 
 VectorValues[i] *= scalar;
 
 return this;
 

 public VectorX Abs()
 
 for (int i = 0; i < VectorCount; i++)
 
 VectorValues[i] = (VectorValues[i] < 0) ? VectorValues[i] *= -1 : VectorValues[i];
 
 return this;
 

 public float Dist(VectorX other)
 
 if (other.VectorCount != this.VectorCount) throw new Exception("Vectors are not of same length");
 else
 
 float sqrtValue = 0;
 for (int i = 0; i < this.VectorCount; i++)
 
 sqrtValue += MathF.Pow((this.VectorValues[i] - other.GetAt(i)), 2);
 
 return MathF.Sqrt(sqrtValue);
 
 

 public VectorX Min(VectorX min)
 
 if (min.VectorCount != this.VectorCount) throw new Exception("Vectors are not of same length");
 else
 
 for (int i = 0; i < this.VectorCount; i++)
 
 this.VectorValues[i] = (this.VectorValues[i] > min.GetAt(i)) ? this.VectorValues[i] : min.GetAt(i);
 
 
 return this;
 

 public VectorX Max(VectorX max)
 
 if (max.VectorCount != this.VectorCount) throw new Exception("Vectors are not of same length");
 else
 
 for (int i = 0; i < VectorCount; i++)
 
 VectorValues[i] = (VectorValues[i] < max.GetAt(i)) ? VectorValues[i] : max.GetAt(i);
 
 
 return this;
 

 public VectorX Clamp(VectorX min, VectorX max)
 

 public VectorX Lerp(VectorX to, float amount)
 
 if (to.VectorCount != VectorCount) throw new Exception("Vectors have different length");
 else
 
 amount = Math.Clamp(amount, 0, 1);
 for (int i = 0; i < VectorCount; i++)
 
 VectorValues[i] = (VectorValues[i] - to.GetAt(i)) * amount + to.GetAt(i);
 
 return this;
 

 

 public float Dot(VectorX other)
 
 if(other.VectorCount != VectorCount) throw new Exception("Vectors are not the same length");
 else
 float d = 0;
 for (int i = 0; i < VectorCount; i++)
 
 d += VectorValues[i] * other.GetAt(i);
 
 return d;
 

 


 public static VectorX NewUnit(int size)
 
 return new VectorX(size, 1);
 
 public static VectorX NewUnitIndex(int size, int index)
 
 VectorX vec = new VectorX(size);
 vec.SetAt(index, 1);
 return vec;
 


 public static float Length(VectorX vector)
 
 return vector.Length();
 

 public static VectorX Normalize(VectorX vector)
 
 return vector.Normalize();
 

 public static VectorX Abs(VectorX vector)
 
 return vector.Abs();
 

 public static float Dist(VectorX vector1, VectorX vector2)
 
 return vector1.Dist(vector2);
 

 public static VectorX Min(VectorX vector, VectorX min)
 
 return vector.Min(min);
 

 public static VectorX Max(VectorX vector, VectorX min)
 
 return vector.Max(min);
 

 public static VectorX Clamp(VectorX vector, VectorX min, VectorX max)
 
 return vector.Clamp(min, max);
 

 public static VectorX Lerp(VectorX vector, VectorX to, float amount)
 
 return vector.Lerp(to, amount);
 

 public static float Dot(VectorX vector1, VectorX vector2)
 
 return vector1.Dot(vector2);
 


 public static VectorX operator +(VectorX left, float value)
 
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) + value);
 
 return left;
 

 public static VectorX operator +(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) + right.GetAt(i));
 
 return left;
 
 

 public static VectorX operator -(VectorX left)
 
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) * -1);
 
 return left;
 

 public static VectorX operator -(VectorX left, float value)
 
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) - value);
 
 return left;
 

 public static VectorX operator -(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) - right.GetAt(i));
 
 return left;
 
 

 public static VectorX operator /(VectorX left, float value)
 
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) / value);
 
 return left;
 

 public static VectorX operator /(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) / right.GetAt(i));
 
 return left;
 
 

 public static VectorX operator *(VectorX left, float value)
 
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) * value);
 
 return left;
 

 public static VectorX operator *(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else
 for (int i = 0; i < left.VectorCount; i++)
 
 left.SetAt(i, left.GetAt(i) * right.GetAt(i));
 
 return left;
 
 

 // do these size comparisions make sense?
 public static bool operator <(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else return (left.Length() < right.Length()) ? true : false;
 

 public static bool operator >(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else return (left.Length() > right.Length()) ? true : false;
 

 public static bool operator >=(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else return (left.Length() >= right.Length()) ? true : false;
 

 public static bool operator <=(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else return (left.Length() <= right.Length()) ? true : false;
 
 // end question

 public static bool operator ==(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else 
 
 for (int i = 0; i < left.VectorCount; i++)
 
 if(left.GetAt(i) != right.GetAt(i)) return false;
 
 return true;
 
 

 public static bool operator !=(VectorX left, VectorX right)
 
 if(left.VectorCount != right.VectorCount) throw new Exception("Vectors not same length");
 else 
 
 for (int i = 0; i < left.VectorCount; i++)
 
 if(left.GetAt(i) != right.GetAt(i)) return true;
 
 return false;
 
 
 

You are inconsistent with your use of the this. 'suffix'. Nobody can agree on this, but you should try to be consistent without projects.

I would remove the return values from every method which modifies Vector3: you have 2 copies of Min, and one is very confusing. All of the methods which happen to produce a vector (e.g. Abs, Min, Max) happen to modifythisand then returnthis. This is, in my opinion, a terrible API, and inconsistent with operations (likeLength) which just happen to _not_ produce a vector. Anyone looking at the signature will assume it does not modify the vector, and you just open yourself to all the nightmares associated with mutablestruct`s. Do not mix an immutable/mutable API like you are now, because it will only infuriate your consumers.

Methods like UpdateFromArray are for some reason returning this as well.

You seem to be trying to provide a consistent API between VectorX (which I would just call Vector) and Vector3, so you might consider an IVector interface.

VectorCount is a mildly confusing name. It's the element count, or the length of the vector.

I would suggest not using Math.Pow(, 2) just to square something: even if it is optimised to detect this exact case (I don't know), it is just harder to read than X * X. I would rewrite Dist as (this - other).length for the sake of simplicity.

It's also common to provide a LengthSquared member which returns the sum before the squareroot, since often this is all that is needed and saves an expensive operation.

The parameter names for Min and Max are odd: other would be fine, but again I don't like the API.

I would expect public Vector3(float[] arr) to throw a nice exception if arr is null, or had a length other than 3.

It's good that most of your methods in VectorX are performing range checks (I didn't notice any that didn't). You don't need to put the 'non-exceptional' code in an else for these.

VectorX has many constructors which just provide 'defaults' for others. I would make these call directly the more general versions. E.g.

public VectorX(int vectorCount) : this(vectorCount, default(float))
 

public VectorX(params float[] values) : this(values, 0, values.Length)
 

This will significantly reduce redundancy and so improve maintainability.

Your ToString methods could be nicer: I would use string interpolation for Vector3 (i.e. $"<X, Y, Z>") and you should use a StringBuilder for VectorX (currently VectorX.ToString() is a quadratic memory operation when it should be linear).

// do these size comparisions make sense?

No, I would say no; though, it does atleast provide an ordering, so it could be much worse.

The comparisons are also performing 2 unnecessary square-roots (you could compare the LengthSquared).

All your Vector3 comparisions also include a completely redundant ternay clause, which will just get in the way of maintaince efforts and provide a greater surface overwhich bugs can will appear.

I would consider describing negatives in terms of the positives, e.g.

public static bool operator !=(Vector3 left, Vector3 right)

 return !(left == right);

Why isn't VectorX.Equals(VectorX other) implemented? What is the point of declaring you implement IEquatable<VectorX> if you do not?

I don't like all your single-line ifs and elses. Even if you don't want to add braces, a line at the end of the condition or else helps significantly with readbility, and reduces the amount of code which is 'off side'.

All of your types and methods would benefit from inline documention (///). This would help to explain the confusing bits of the API, and clarify what methods like Length and Normalise mean.

VectorValues[i] = (VectorValues[i] < 0) ? VectorValues[i] *= -1 :
VectorValues[i];

What is wrong with Math.Abs(VectorValues[i]).

public int VectorCount get return 3;

This can be made a little more concise:

public int VectorCount => 3;

You could do the same with the static Unit and Zero members.

You have some odd line-spacing in places (e.g. around VectorX.NewUnit). I can see no reason for this, so it just looks untidy and makes the code harder to scan.

Consider using an indexer instead of the SetAt and GetAt methods. These could be part of the IVector interface also.

VectorX.VectorValues should not be mutable, and probably shouldn't be public. At the moment it is possible for someone to change VectorValues such that VectorCount will be wrong, or even to change it null and wreck everything. Hide it away so that people can't shoot themselves in the foot with your API.

You could re-write VectorX.VectorCount in terms of VectorValues to reduce redunancy and simplify the constructors.

public int VectorCount => VectorValues.Length;

Less redundancy again means there is less to go wrong, which makes the code easier to maintain.

The VectorX(VectorX) constructor is not good: all the other constructors duplicate the array they are given, but this just copies the reference. There is no point in this constructor (it's the same as a value-copy) and it will only create confusion. If instead it copied the array, then it would be fine.

public VectorX(VectorX vector) : this(vector.VectorValues)
 

ToArray would also imply a copy, and with VectorValues public does nothing useful. It should take a copy. This is as easy as VectorValues.ToArray() if you have using System.Linq at the top of your file. The fact that it does/doesn't copy should be in the documention.

I'm going to stop for now, but I may add to this answer when I have time, though I'm sure someone else will take up the slack long before then if there is more to be said.