9
\$\begingroup\$

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)
        {
            return (left.X != right.X || left.Y != right.Y || 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 || pos < 0) throw new Exception("Supplied position is too large or less then 0");
            else return VectorValues[pos - 1];
        }

        public VectorX SetAt(int pos, float value)
        {
            if (pos >= VectorCount || 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)
        {
            if (max.VectorCount != VectorCount || min.VectorCount != VectorCount) throw new Exception("Vectors are not of same length");
            else
            {
                for (int i = 0; i < VectorCount; i++)
                {
                    VectorValues[i] = (VectorValues[i] < min.GetAt(i)) ? min.GetAt(i) : VectorValues[i] = (VectorValues[i] > max.GetAt(i)) ? max.GetAt(i) : VectorValues[i];
                }
                return this;
            }
        }

        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;
            }
        }
    }
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ This seems alot like a javascript library (three.js) ported to C#. \$\endgroup\$
    – dfhwze
    Sep 18, 2019 at 18:03
  • 1
    \$\begingroup\$ Have you considered using Vector class from System.Numerics? \$\endgroup\$
    – Rick Davin
    Sep 18, 2019 at 18:09
  • 3
    \$\begingroup\$ Could you name the limitations? \$\endgroup\$
    – t3chb0t
    Sep 19, 2019 at 10:34
  • 2
    \$\begingroup\$ Welcome to Code Review! Please see What to do when someone answers. I have rolled back Rev 6 → 3 \$\endgroup\$ Sep 19, 2019 at 21:30
  • 1
    \$\begingroup\$ @t3chb0t I will try to explain what i was missing in the System.numerics namespace. First of all i wanted to be able to implicitly convert vectors between types to go from Vector3 to Vector2, I wanted some extra constructors, A bunch of methods that don't exist in the Numerics vector. And A vector of user defined length, Generics that i still have to make :) And probably some other stuff i cant think of right now... And most of all it is a great learning exercise :) \$\endgroup\$
    – FutureCake
    Sep 20, 2019 at 21:17

2 Answers 2

13
\$\begingroup\$

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.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ It will be hard writing a complement answer without having some kind of intersection with this answer :p \$\endgroup\$
    – dfhwze
    Sep 18, 2019 at 17:55
  • 1
    \$\begingroup\$ About the status of Pow(x, 2), it is not (yet) optimized by the main implementations, there is an open issue for coreclr, so maybe someday \$\endgroup\$
    – harold
    Sep 18, 2019 at 18:31
  • \$\begingroup\$ VisualMelon, Thank you so much for the extensive list of improvements!!! As you may have seen from my code i am no pro ;) But this sure as hell will help with my improvement of skills :) I will be working on this list in the next few days probably :) Please keep adding indeed if you feel like it :) \$\endgroup\$
    – FutureCake
    Sep 18, 2019 at 18:44
  • 1
    \$\begingroup\$ You didn't mention the new readonly struct, or I missed it. This would cure all of the mutability issues already on the compile level. \$\endgroup\$
    – t3chb0t
    Sep 19, 2019 at 10:36
  • 1
    \$\begingroup\$ @t3chb0t I had a section about mutability, but frankly that would remove most of the API (so I removed it). Please do write your own answer about that (if you want), because I won't be adding it. \$\endgroup\$ Sep 19, 2019 at 10:44
5
\$\begingroup\$

Ok, I think I found something noteworthy not yet mentioned by VisualMelon.

You are already using MathF to perform some operations for you, why not redirect some others to Math?

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;
}

could be written as:

public Vector3 Min(Vector3 min)
{
    this.X = Math.Min(this.X, min.X);
    this.Y = Math.Min(this.Y, min.Y);
    this.Z = Math.Min(this.Z, min.Z);
    return this;
}

You could do the equivalent for method Max.

Note an inconsistency or at least a very confusing meaning of the variables min and max. In methods Min and Max you take the min, respectively max of the current value with the provided value. While in Clamp you turn the meaning of min and max around.

I would refactor Min and Max to use the same definition for min and max as Clamp. This way, the provided variable is a boundary, just as in Clamp.

public Vector3 Min(Vector3 max)
{
    this.X = Math.Min(this.X, max.X);
    this.Y = Math.Min(this.Y, max.Y);
    this.Z = Math.Min(this.Z, max.Z);
    return this;
}

public Vector3 Max(Vector3 min)
{
    this.X = Math.Max(this.X, min.X);
    this.Y = Math.Max(this.Y, min.Y);
    this.Z = Math.Max(this.Z, min.Z);
    return this;
}

Clamp could then be rewritten (you could also add some validation that max > min):

public Vector3 Clamp(Vector3 min, Vector3 max)
{
    this.Max(min);
    this.Min(max);
    return this;
}

One other thing about the original Clamp code:

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;
}

You don't have to assign this.* to itself in the ternary operations. You could simplify to:

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

Or a bit more compact using Math:

public Vector3 Clamp(Vector3 min, Vector3 max)
{
    this.X = Math.Min(Math.Max(this.X, min.X), max.X);
    this.Y = Math.Min(Math.Max(this.Y, min.Y), max.Y);
    this.Z = Math.Min(Math.Max(this.Z, min.Z), max.Z);
    return this;
}

Beyond this, I would also favor to use immutable classes. So instead of changing the instance coordinates, I would create a new instance of a Vector3 instead. If the entire API is written this way, your fluent methods make sense and would consistently return new instances on chaining.

\$\endgroup\$
2
  • \$\begingroup\$ +1 Thanks for the advice! Just one question about the using of Math.Min, Math.Max etc. Isn't this less efficient because then i am first calling an other class which calls a method and then returns the result? Instead of just doing it with statements? Or is the difference negligible? \$\endgroup\$
    – FutureCake
    Sep 20, 2019 at 10:42
  • 2
    \$\begingroup\$ The performance penalty of calling another method is negligible, as you say :) usability and reusability should definately be favored here \$\endgroup\$
    – dfhwze
    Sep 20, 2019 at 11:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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