Skip to content

Add copy method for handles#306

Merged
ctrueden merged 3 commits intomasterfrom
datahandles-copy
Nov 9, 2017
Merged

Add copy method for handles#306
ctrueden merged 3 commits intomasterfrom
datahandles-copy

Conversation

@gab1one
Copy link
Contributor

@gab1one gab1one commented Nov 7, 2017

Adds a new method to the DataHandles class that allows for easy copying using an internal buffer

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Looks great! I wrote some nitpicks. Please fix, and we can merge it!

Copy link
Member

Choose a reason for hiding this comment

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

Let's use byte array handles, instead of files. No need to waste disk I/O for this one.

Copy link
Member

Choose a reason for hiding this comment

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

Kill this blank line.

Copy link
Member

Choose a reason for hiding this comment

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

You use underscore in numeric constant above, but not here. Let's be consistent!

Copy link
Member

Choose a reason for hiding this comment

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

Let's make this non-static. I.e.: spin a new context in @Before rather than using @BeforeClass and potentially bleeding state between tests.

@@ -0,0 +1,135 @@

Copy link
Member

Choose a reason for hiding this comment

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

Missing license header.

Copy link
Member

Choose a reason for hiding this comment

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

Need to write when the exception is thrown.

Copy link
Member

Choose a reason for hiding this comment

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

Should state here that 0 means all bytes, same as in the other signature.

*
* @param in input handle
* @param out the output handle
* @param length maximum number of bytes to copy, will copy all bytes if set
Copy link
Member

Choose a reason for hiding this comment

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

More grammatically correct would be ; instead of ,. Otherwise it's a comma splice.

Copy link
Member

Choose a reason for hiding this comment

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

You can achieve final on this variable if you structure this section a little more carefully.

Copy link
Member

Choose a reason for hiding this comment

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

Should use curly braces for the consequent here, so formatter does the right thing.

@gab1one
Copy link
Contributor Author

gab1one commented Nov 8, 2017

@ctrueden I updated the pr with all the changes you requested.

@ctrueden ctrueden merged commit b8e772a into master Nov 9, 2017
@ctrueden
Copy link
Member

ctrueden commented Nov 9, 2017

Thanks!!

@ctrueden ctrueden deleted the datahandles-copy branch November 9, 2017 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments