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 2 commits
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
29 changes: 29 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,35 @@ internal PdfVector ToVector()
return new PdfVector(X, Y);
}

/// <summary>
/// Converts this <see cref="PdfPoint"/> into an double array.
/// </summary>
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>
public override bool Equals(object obj)
{
if (obj is PdfPoint point)
{
return point.X == this.X && point.Y == this.Y;
}
return false;
}

/// <summary>
/// Returns the hash code for this <see cref="PdfPoint"/>.
/// </summary>
public override int GetHashCode()
{
return (X, Y).GetHashCode();
}

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