Classes representing 2D points and pixels

Question

How can I improve this code or make it tidier?

public class Point2d : IEquatable<Point2d> { public double X { get; set; } public double Y { get; set; } #region constructor public Point2d(double x, double y) { X = x; Y = y; } public Point2d(double [] points) { if (points.GetLength(0) < 2) throw new Exception("[in Point2d(double [] points)] the array must be at least 2 elements long."); X = points[0]; Y = points[1]; } #endregion public void Print() { Console.Write("("); Console.Write(X); Console.Write(","); Console.Write(Y); Console.Write(") "); } public double GetDistance(Point2d otherPoint) { return Math.Sqrt(GetSquaredDistance(otherPoint)); } public double GetSquaredDistance(Point2d otherPoint) { return ((otherPoint.X - X) * (otherPoint.X - X)) + ((otherPoint.Y - Y) * (otherPoint.Y - Y)); } public Point2d GetTranslated(double x, double y) { return GetTranslated(new Point2d(x, y)); } public Point2d GetTranslated(Point2d center) { return new Point2d(X + center.X, Y + center.Y); } #region override string ToString() public override string ToString() { StringBuilder sb = new StringBuilder(); sb.AppendFormat("({0:0.00}, {1:0.00})", X, Y); return sb.ToString(); } #endregion #region equality comparison implementations public override bool Equals(object other) { if (!(other is Point2d)) return false; return Equals((Point2d)other); } public bool Equals(Point2d other) { return Math.Round(X, 2) == Math.Round(other.X, 2) && Math.Round(Y,2) == Math.Round(other.Y, 2); } public override int GetHashCode() { return (int)Math.Round(Y * 31.0 + X, 0); // 31 = some prime number } public static bool operator ==(Point2d a1, Point2d a2) { return a1.Equals(a2); } public static bool operator !=(Point2d a1, Point2d a2) { return !a1.Equals(a2); } #endregion } public class Pixel2d : Point2d, IEquatable<Pixel2d> { public int State { get; set; } public Pixel2d(): this(0,0,0) { } public Pixel2d(int x, int y, int State) : base(x, y) { this.State = State; } #region equality comparison implementations public override bool Equals(object other) { if (!(other is Pixel2d)) return false; return Equals((Pixel2d)other); } public bool Equals(Pixel2d other) { return Math.Round(X, 2) == Math.Round(other.X, 2) && Math.Round(Y, 2) == Math.Round(other.Y, 2) && State == other.State; } public override int GetHashCode() { return (int)Math.Round(Y * 31.0 + X, 0); // 31 = some prime number } public static bool operator ==(Pixel2d a1, Pixel2d a2) { return a1.Equals(a2); } public static bool operator !=(Pixel2d a1, Pixel2d a2) { return !a1.Equals(a2); } #endregion } 

For example:

1. Is it possible to merge IEquatable<> implementations into a single generic function or separate them into another class?
2. Is there any better way to write 'Point2d and Pixel2d parameterized constructors?
3. What else can be made tidier?

Answer 1

A fine example how modern C# can make your life so much easier! But first things first:

Errors and maybe errors

Your GetHashCode() is realy weak! Just take two example points (1|1) and (0|32) and compare their hashcode!

Also your Point2d uses doubles for X and Y while Pixel2d runs on int, no technical issue because of implicit conversions but maybe not what youre looking for.

Point2d.Equals(object other) will crash if you pass in null.

X and Y are setable which is most likely not useful in any given scenario.

I can't see the use case for the Point2d constructor taking an double[]. If you realy need it, atleast make it explicit and check for a length of exactly 2, instead of taking anything big enough.

You allow an parameterless constructor on Pixel2d but disallow it for Point2d.

Equals() look rather suspicious due to its rounding.

Codestyle

I do not see the benefit of regions here, so i'd rather remove them. Single line if-brackets are ommited while unnecessary ones for arithmetic expressions are included. I prefer always settings them for scopes and include arithmetic ones if its getting to complex, but thats personal preference after all.

Read up on string interpolation and your ToString() can be a single line

 return $"({X:0.00}, {Y:0.00})"; // Note the leading $ 

Your Print() could be a single line of

Console.Write($"({X},{Y}) "); 

Point2D.Equals() can be simplified to

public override bool Equals(object other) { return other is Point2d point2d && Equals(point2d); } 

Modern C#

Beside string interpolation modern C# has way more tools to help you keep your code clean. This is an prime example why there are socalled "Records", capable of auto implementing Equals(), GetHashCode() and many more! Another term to look up is "Primary Constructors" and maybe you also want to check for nullable context, to easily spots errors like that nullreference exception on your Equals().

Simplified

Using all these tools your code could look something like this:

public record Point2d(double X, double Y) { public void Print() { Console.Write($"({X},{Y}) "); } public double GetDistance(Point2d otherPoint) { return Math.Sqrt(GetSquaredDistance(otherPoint)); } public double GetSquaredDistance(Point2d otherPoint) { return (otherPoint.X - X) * (otherPoint.X - X) + (otherPoint.Y - Y) * (otherPoint.Y - Y); } public Point2d GetTranslated(double x, double y) { return GetTranslated(new Point2d(x, y)); } public Point2d GetTranslated(Point2d center) { return new Point2d(X + center.X, Y + center.Y); } public override string ToString() { return $"({X:0.00}, {Y:0.00})"; } public virtual bool Equals(Point2d? other) { return other != null && Math.Round(X, 2) == Math.Round(other.X, 2) && Math.Round(Y, 2) == Math.Round(other.Y, 2); } } 

And

public record Pixel2d(double X, double Y, int State) : Point2d(X, Y) { public int State { get; set; } = State; public virtual bool Equals(Pixel2d? other) { return base.Equals(other) && State == other.State; } } 

Hope that helps!

Answer 2

A few other things:

• Soon (C#11/.NET 7) you can change the type from int and double to a generic type <T> using INumber

• It's a bit weird adding Print. Not the concern of the Point class to say where it should be printed. Better to let the caller do it and just use ToString.

• Instead of hard coding number format in ToString, use IFormattable and use format strings yourself.

• If you really want to initialize the point using an array, taking an IEnumerable would make sense instead. (And in C# 11 you can use List Patterns)

• I would add rather add an IsCloseEnough(Point2d otherPoint, double precision = 0.01) than hard code it in the methods

A more "enterpricey" thing you can do (and somewhat dubious) would be to extract the Point2D into an interface IPoint2D.

The reason for that would be to, for example, have two versions of the point, where one is simple, but the other caches the more expensive calculations.

---

Finally, I would not use inheritance for the pixel. This is of course a matter of discussion (and situation specific) but the default should always start with composition rather than inheritance. The Pixel "is not" a Point, but it "has a point".

Also, I would probably make it generic to be able to represent many kinds of pixels:

record Pixel2d<T>(Point2d point, T state) { ... }