Improving PdfPoint

[BobLd]

Adding:

• ToDouble
• Equals
• GetHashCode

Not sure if GetHashCode is properly done, any feedback welcome.

[EliotJones]

Thanks for adding these to PdfPoint, they're definitely needed and should help consumers out. I'd recommend changing the ToDouble return type but I'm aware this might have knock-on impacts for any code you were working on that needed an array. Let me know if it's unworkable for you and maybe we can have the array based implementation as an extension method,

[EliotJones]

There's a slightly terser syntax for this:

Suggested change

	if (obj is PdfPoint)
	if (obj is PdfPoint point)

Then you can remove line 103, but I don't mind, I just know Resharper is going to nag me 😆

[BobLd]

Very good, thx!

[EliotJones]

I wonder if this would be better as a ValueTuple rather than an array. Since the array will always be known to have 2 members a tuple has a couple of benefits:

• Prevents incorrect indexing by consumers
• Avoids an allocation on the heap which needs garbage collection

This would look something like:

Suggested change

	public double[] ToDouble()
	/// <summary>Converts this <see cref="PdfPoint" to double values.<.summary>
	public (double x, double y) ToDouble()

I think PdfPig already includes ValueTuple as a dependency (it's a nuget package for earlier versions of the .NET Framework) so all consumers will be able to use it.

(secondary nitpick on the empty <returns> tags).

[BobLd]

On this, I would really need a double array for computation purpose...

[EliotJones]

Hmm, I'd be tempted to leave this off the public API for now. My gut instinct is that it'll confuse consumers of the library over the alternative of having a tuple. You could add:

public static class PdfPointExtensions
{
    public static double[] AsDoubleArray(this PdfPoint point) => new [] { (double)point.X, (double)point.Y };
}

Either to your consumer code or to PdfPig (or internally to PdfPig if it's for layout analysis code internally) but I'd avoid having it as a method directly on the public API of PdfPoint for now to reduce the amount of autocomplete options.

[BobLd]

okay, you are right. Let's keep it simple and leave it out for the moment

[EliotJones]

Overriding GetHashCode is one of those annoying things that we're told to do but correct approaches to it aren't really documented anywhere. There's a couple of discussions on it:

• https://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode/263416#263416
• https://blogs.msdn.microsoft.com/ericlippert/2011/02/28/guidelines-and-rules-for-gethashcode/

Initially I thought the approach here might be more vulnerable to collisions but now I'm not so sure, though I guess the only way to be sure is calculate the collision rate or something. I tend to treat hash codes as something like encryption in that they're much easier to use library functions for and easy to get wrong when written from scratch.

I generally rely on Resharper when doing this which generates something that closely matches Jon Skeet's answer. For reference this looks something like:

public override int GetHashCode()
{
    var hashCode = 1861411795;
    hashCode = hashCode * -1521134295 + X.GetHashCode();
    hashCode = hashCode * -1521134295 + Y.GetHashCode();
    return hashCode;
}

The other option is to let the .NET Framework do the heavy lifting by relying on the ValueTuple implementation:

return (X, Y).GetHashCode();

Which turns the point into a ValueTuple and uses the implementation from that which is here: https://github.com/dotnet/roslyn/blob/master/src/Compilers/Test/Resources/Core/NetFX/ValueTuple/ValueTuple.cs#L297. Now I'm not sure what that does but I'd hope it's one of the best possible implementations.

[BobLd]

Hehe exactly. Let's .Net do the job then!