Port of Java PR #267 - Clipping Path Name / Path Information Extraction #235
Conversation
| /// </summary> | ||
| /// <param name="index">location of point to be added in points</param> | ||
| /// <param name="point">coordinate value to be added to points</param> | ||
| public void SetPoint(int index, double point) |
There was a problem hiding this comment.
These get and set methods could be replaces with an indexer property if you liked (not available in Java). Up to you.
| /// Gets size of knots list | ||
| /// </summary> | ||
| /// <returns>size of knots List</returns> | ||
| public int Size() |
There was a problem hiding this comment.
This could be a property. Alternatively, remove this Size member, make GetKnots an IReadOnlyList<Knot> and allow callers to use subPath.Knots.Count, which would feel more idiomatic in .NET. The name "size" is more Java-esque.
There was a problem hiding this comment.
That works, until NET3.5 compat kicks in - then it gets a little weird. I might change the name from GetKnots to Knots and "Size" to "KnotsCount". Otherwise, it has to be the Count() Linq method, which will cause an enumeration. I'll post it and see what you think.
| /// <author>Kevin Mott https://github.com/kwhopper</author> | ||
| public class Subpath | ||
| { | ||
| private List<Knot> p_knots = new List<Knot>(); |
There was a problem hiding this comment.
| private List<Knot> p_knots = new List<Knot>(); | |
| private readonly List<Knot> p_knots = new List<Knot>(); |
- remove 'set' from Type property in Knot and Subpath classes - replace Get/Set point methods with indexer in Knot class - make some private variables readonly - replace GetKnots() method with Knots property in Subpath - replace Size() method with KnotCount property in Subpath
|
Tried to accommodate all your suggestions. Thanks |
| @@ -71,16 +72,22 @@ public DirectoryList Extract(SequentialReader reader, int length) | |||
| pos += 2; | |||
|
|
|||
| // A variable number of bytes holding a pascal string (two leading bytes for length). | |||
| var descriptionLength = reader.GetByte(); | |||
| int descriptionLength = reader.GetByte(); | |||
There was a problem hiding this comment.
| int descriptionLength = reader.GetByte(); | |
| byte descriptionLength = reader.GetByte(); |
There was a problem hiding this comment.
I'd use the actual type here rather that widening to a signed type. It'll prevent wondering whether it may be less than zero below.
- remove Int operation (adding pos) on descriptionLength to avoid casting issues - use descriptionLength decrementing logic instead of adding pos first - fix regression in setting clippingpath tag
|
Removed the int cast of descriptionLength. It initially had to be int because the Java code adds pos to it before entering the loop so it was either start with an int or cast pos. I removed that logic and used a decrement of descriptionLength in the loop instead. Also fixed a regression when using the new clippingpath constants. If this looks ok to you, I'll go ahead and merge. Thanks! |
|
Thanks again. |
port of Java PR drewnoakes/metadata-extractor#267