Skip to content

Improving PdfPoint #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
EliotJones merged 3 commits into from
Aug 10, 2019
Merged

Improving PdfPoint #54

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
474ce9a
Improving PdfPoint
Aug 9, 2019
bd58879
Update from comments
Aug 10, 2019
9b24223
Removing ToDouble()
Aug 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/UglyToad.PdfPig/Geometry/PdfPoint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,39 @@ internal PdfVector ToVector()
return new PdfVector(X, Y);
}

/// <summary>
/// Converts this <see cref="PdfPoint"/> into an array.
/// </summary>
/// <returns></returns>
public double[] ToDouble()

@EliotJones EliotJones Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 BobLd Aug 10, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@EliotJones EliotJones Aug 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 BobLd Aug 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
return new double[] { (double)this.X, (double)this.Y };
}

/// <summary>
/// Returns a value indicating whether this <see cref="PdfPoint"/> is equal to a specified <see cref="PdfPoint"/> .
/// </summary>
/// <param name="obj"></param>
/// <returns></returns>
public override bool Equals(object obj)
{
if (obj is PdfPoint)

@EliotJones EliotJones Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 BobLd Aug 10, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good, thx!

{
PdfPoint point = (PdfPoint)obj;
return point.X == this.X && point.Y == this.Y;
}
return false;
}

/// <summary>
/// Returns the hash code for this <see cref="PdfPoint"/>.
/// </summary>
/// <returns></returns>
public override int GetHashCode()
{
return (int)(this.X * 10_000 + 31 * this.Y * 10_000);

@EliotJones EliotJones Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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 BobLd Aug 10, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/// <summary>
/// Get a string representation of this point.
/// </summary>
Expand Down