3
\$\begingroup\$

I built a couple Vector2 structures (Vector2 and Vector2F) for use with a particular project, so I'm mostly requesting best practices here.

The biggest concern is that I'm not making any assumptions I shouldn't be. I would like any comments that are relevant on the interface and whether it's too light, or if it's just right.

Do note: I know they should be auto-implemented properties where possible, but in this particular case I really don't want to use them like that.

public interface IVector2
{
    double R { get; }
    double Theta { get; }
    bool IsEmpty { get; }
}

public struct Vector2 : IVector2
{
    private int _x;
    private int _y;

    public Vector2(int x, int y) { _x = x; _y = y; }

    [BrowsableAttribute(false)]
    public bool IsEmpty { get { return this == Empty; } }

    public int X { get { return _x; } private set { _x = value; } }
    public int Y { get { return _y; } private set { _y = value; } }
    public double R { get { return Math.Sqrt((double)_x * _x + (double)_y * _y); } }
    public double Theta { get { return Math.Atan2((double)_y, (double)_x); } }

    public static Vector2 Add(Vector2 v1, Vector2 v2) { return v1 + v2; }
    public static Vector2 Ceiling(Vector2F value) { return new Vector2((int)Math.Ceiling(value.X), (int)Math.Ceiling(value.Y)); }
    public override bool Equals(Object obj) { if (obj is Vector2) { return (Vector2)obj == this; } else { return false; } }

    public override int GetHashCode()
    {
        int hash = 17;

        hash = hash * 23 + _x.GetHashCode();
        hash = hash * 23 + _y.GetHashCode();

        return hash;
    }

    public static Vector2 Round(Vector2F value) { return new Vector2((int)Math.Round(value.X), (int)Math.Round(value.Y)); }
    public static Vector2 Subtract(Vector2 v1, Vector2 v2) { return v1 - v2; }
    public override string ToString() { return string.Format("({0},{1})", _x, _y); }
    public static Vector2 Truncate(Vector2F value) { return new Vector2((int)(value.X), (int)(value.Y)); }
    public static Vector2 FromRTheta(double r, double theta) { return new Vector2((int)(r * Math.Cos(theta)), (int)(r * Math.Sin(theta))); }

    public static Vector2 operator +(Vector2 v1, Vector2 v2) { return new Vector2(v1.X + v2.X, v1.Y + v2.Y); }
    public static bool operator ==(Vector2 left, Vector2 right) { return left.X == right.X && left.Y == right.Y; }
    public static implicit operator Vector2F(Vector2 p) { return new Vector2F(p.X, p.Y); }
    public static bool operator !=(Vector2 left, Vector2 right) { return left.X != right.X || left.Y != right.Y; }
    public static Vector2 operator -(Vector2 v1, Vector2 v2) { return new Vector2(v1.X - v2.X, v1.Y - v2.Y); }

    public static readonly Vector2 Empty = new Vector2(0, 0);
}

public struct Vector2F : IVector2
{
    private float _x;
    private float _y;

    public Vector2F(float x, float y) { _x = x; _y = y; }

    [BrowsableAttribute(false)]
    public bool IsEmpty { get { return this == Empty; } }

    public float X { get { return _x; } private set { _x = value; } }
    public float Y { get { return _y; } private set { _y = value; } }
    public double R { get { return Math.Sqrt((double)_x * _x + (double)_y * _y); } }
    public double Theta { get { return Math.Atan2((double)_y, (double)_x); } }

    public static Vector2F Add(Vector2F v1, Vector2 v2) { return v1 + v2; }
    public static Vector2F Add(Vector2F v1, Vector2F v2) { return v1 + v2; }
    public override bool Equals(Object obj) { if (obj is Vector2F) { return (Vector2F)obj == this; } else { return false; } }

    public override int GetHashCode()
    {
        int hash = 17;

        hash = hash * 23 + _x.GetHashCode();
        hash = hash * 23 + _y.GetHashCode();

        return hash;
    }

    public static Vector2F Subtract(Vector2F v1, Vector2 v2) { return v1 - v2; }
    public static Vector2F Subtract(Vector2F v1, Vector2F v2) { return v1 - v2; }
    public override string ToString() { return string.Format("({0},{1})", _x, _y); }
    public static Vector2F FromRTheta(double r, double theta) { return new Vector2F((float)(r * Math.Cos(theta)), (float)(r * Math.Sin(theta))); }
    public Vector2F Invert() { return new Vector2F(0 - _x, 0 - _y); }

    public static Vector2F operator +(Vector2F v1, Vector2 v2) { return new Vector2F(v1.X + v2.X, v1.Y + v2.Y); }
    public static Vector2F operator +(Vector2F v1, Vector2F v2) { return new Vector2F(v1.X + v2.X, v1.Y + v2.Y); }
    public static bool operator ==(Vector2F left, Vector2F right) { return left.X == right.X && left.Y == right.Y; }
    public static bool operator !=(Vector2F left, Vector2F right) { return left.X != right.X || left.Y != right.Y; }
    public static Vector2F operator -(Vector2F v1, Vector2 v2) { return new Vector2F(v1.X - v2.X, v1.Y - v2.Y); }
    public static Vector2F operator -(Vector2F v1, Vector2F v2) { return new Vector2F(v1.X - v2.X, v1.Y - v2.Y); }

    public static readonly Vector2F Empty = new Vector2F(0, 0);
}

Also, example usage of the IVector2 interface:

public static class VectorExtensions
{
    public static Direction? GetDirection(this IVector2 vector)
    {
        if (vector.R <= 0)
            return null;

        double theta = vector.Theta;

        if (Math.Abs(theta) <= Math.PI * 0.25f)
            return Direction.Right;

        if (Math.Abs(theta) >= Math.PI * 0.75f)
            return Direction.Left;

        if (theta >= Math.PI * 0.25f && theta < Math.PI * 0.75f)
            return Direction.Down;

        return Direction.Up;
    }
}

And the usage of that extension method:

        vector = Vector2F.FromRTheta(r, vector.Theta);

        Direction? d = vector.GetDirection();

        if (d.HasValue)
            Direction = d.Value
\$\endgroup\$
  • \$\begingroup\$ Are you using C# 6.0? \$\endgroup\$ – Mathieu Guindon Aug 25 '15 at 18:46
  • \$\begingroup\$ @Mat'sMug Well, I have access to C#6.0, but at the moment it still has to compile in a C#5.0 environment. (Damn XNA!) If you want to make C#6.0 specific suggestions, I'm all for it. \$\endgroup\$ – Der Kommissar Aug 25 '15 at 18:46
  • \$\begingroup\$ @Mat'sMug Now I have full C#6.0. I managed to finagle XNA into Visual Studio 2015. \$\endgroup\$ – Der Kommissar Aug 25 '15 at 19:24
  • \$\begingroup\$ What's the purpose of the interface? How is it intended to be used? Can you give an example of a usage that would use the implementations interchangeably? \$\endgroup\$ – craftworkgames Aug 26 '15 at 13:39
  • \$\begingroup\$ @craftworkgames Updated question with an example usage. \$\endgroup\$ – Der Kommissar Aug 26 '15 at 13:41
4
\$\begingroup\$
public override int GetHashCode()
{
    int hash = 17;

    hash = hash * 23 + _x.GetHashCode();
    hash = hash * 23 + _y.GetHashCode();

    return hash;
}

Calling GetHashCode() on an int, returns that int. For larger values of _x and _y, you are exposing yourself to overflow errors, unless you're building unchecked. Best is to instruct the compiler to just let the int overflow, resulting in a negative hash code instead of an exception:

public override int GetHashCode()
{
    int hash = 17;
    unchecked
    {
        hash = hash * 23 + _x.GetHashCode();
        hash = hash * 23 + _y.GetHashCode();
    }
    return hash;
}

public int X { get { return _x; } private set { _x = value; } }

I'd suggest one of two things:

  • Make _x and _y be readonly and constructor-initialized
  • Make X and Y be immutable auto-properties (C# 6.0 only)

The private modifier on the setter suggests mutability, which is a bad, bad idea for a value type.


public static Vector2 Add(Vector2 v1, Vector2 v2) { return v1 + v2; }

Why make it static? Couldn't it be an instance method with a single parameter?

Readability-wise, I don't mind one-liner properties.

But one-liner methods (and operator overrides) are a problem. Get rid of that useless horizontal scrolling nightmare, and go vertical!


public static readonly Vector2 Empty = new Vector2(0, 0);

Love it.

\$\endgroup\$
  • \$\begingroup\$ But I have a 1080p monitor, I have all this horizontal space I don't use. If not for codes then why made of big? \$\endgroup\$ – Der Kommissar Aug 25 '15 at 19:27
  • \$\begingroup\$ @EBrown well then, have the solution explorer pinned to the right, and have your unit tests pinned to the left, wide enough to see all the good descriptive method names ;-) \$\endgroup\$ – Mathieu Guindon Aug 25 '15 at 19:29
  • \$\begingroup\$ But then what good are multiple displays? :P \$\endgroup\$ – Der Kommissar Aug 25 '15 at 19:40
  • 2
    \$\begingroup\$ Would you read a book that was so wide you had to turn your head to read the lines? No. The same applies for code and content on the web. It's far easier to read thin columns of text. Newspapers have been doing it for decades. \$\endgroup\$ – craftworkgames Aug 26 '15 at 13:33

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.