Skip to content
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

Jump to bottom

[IO-711] Use Objects.requireNotNull for fail fast method/constructors #196

Closed
wants to merge 1 commit into from
Closed

[IO-711] Use Objects.requireNotNull for fail fast method/constructors #196

wants to merge 1 commit into from

Conversation

boris-unckel
Copy link
Contributor

@boris-unckel
Copy link
Contributor Author

@garydgregory Please review and mark the cases you don't want to change for compatibility and/or design reasons. I'll change then all occurrences accordingly.

garydgregory
garydgregory requested changes Jan 28, 2021
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hi @boris-unckel

Thank you for your PR. Please see my comments.

src/main/java/org/apache/commons/io/FileDeleteStrategy.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/FileSystemUtils.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/FileSystemUtils.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/FileUtils.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/FileUtils.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/FileUtils.java
@@ -866,6 +868,8 @@ public static void copyFile(final File srcFile, final File destFile, final CopyO
* @since 2.1
*/
public static long copyFile(final File input, final OutputStream output) throws IOException {
Objects.requireNonNull(input, "input");
Objects.requireNonNull(output, "output");
Copy link
Member

Choose a reason for hiding this comment

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

-1: no need to check on output, this already done in copyLarge.

I'll stop this type of comment since these checks over the top in this PR. IOW, let's not check for NPEs when we do not need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garydgregory The other cases before I understood the pattern behind "-1". In this case a FileInputStream is created, afterwards copyLarge calls copy calls copyLarge which then does the check.
I'll fix the former cases and wait with removal for cases like this on your comment.

src/main/java/org/apache/commons/io/FileUtils.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/FilenameUtils.java Outdated Show resolved Hide resolved
src/main/java/org/apache/commons/io/monitor/FileAlterationObserver.java Outdated Show resolved Hide resolved
@garydgregory
Copy link
Member

@garydgregory Please review and mark the cases you don't want to change for compatibility and/or design reasons. I'll change then all occurrences accordingly.

Hi @boris-unckel

Sorry but I don't want to take the time to review each change in each of 31 files. If you do not want to revise the PR to minimize changes and duplication of null checks, that's fine as well. I might get to these kinds of changes later. Thank you for your contributions so far though :-)

@boris-unckel boris-unckel mentioned this pull request Jan 29, 2021
Closed
@boris-unckel boris-unckel force-pushed the utilize_objects_requirenotnull branch 2 times, most recently from d429c96 to 8912dd6 Compare January 29, 2021 15:19
@boris-unckel
Copy link
Contributor Author

@garydgregory

  • Fixed the formatting (is there a official eclipse configuration you can recommend?)
  • Removed single parameter checks in case of direct usage,
  • Kept single parameter checks when prior (different) Object creation happens (see case above)
  • Kept multiple parameter checks
  • Removed double checks in constructors, even if objects are created
  • Kept all checks in constructors, where fail will be in post constructor usage of methods

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 89.121% when pulling cb17517 on boris-unckel:utilize_objects_requirenotnull into 8665a70 on apache:master.

@garydgregory
Copy link
Member

@boris-unckel
May you please rebase on master?

@boris-unckel boris-unckel deleted the utilize_objects_requirenotnull branch June 12, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garydgregory garydgregory garydgregory requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@boris-unckel @garydgregory @coveralls