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.

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

Code cleanup and minor fixes #1040

Merged
merged 5 commits into from May 12, 2020
Merged

Code cleanup and minor fixes #1040

merged 5 commits into from May 12, 2020

Conversation

NewbieOrange
Copy link
Contributor

Removed duplicated null checking, fix several potential NullPointerException.

Also fixed NPE when user Ctrl+D in password prompt.

@remkop
Copy link
Owner

remkop commented May 11, 2020

Thank you!
I took a quick look; I will take a more detailed look later.

Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Overall some good catches, but there are some places where I like the current style.
Specifically:

  • I generally think String concatenation with + gives more readable code than StringBuilder.append
  • Using Map.Entry to loop over maps is more efficient, but tends to give longer and less readable code.

src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented May 12, 2020

Codecov Report

Merging #1040 into master will decrease coverage by 0.01%.
The diff coverage is 82.97%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1040      +/-   ##
============================================
- Coverage     94.37%   94.35%   -0.02%     
  Complexity      443      443              
============================================
  Files             2        2              
  Lines          6474     6469       -5     
  Branches       1728     1726       -2     
============================================
- Hits           6110     6104       -6     
- Misses           96       98       +2     
+ Partials        268      267       -1     
Impacted Files Coverage Δ Complexity Δ
src/main/java/picocli/CommandLine.java 94.19% <82.97%> (-0.03%) 304.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e23f7ea...b750534. Read the comment docs.

@NewbieOrange
Copy link
Contributor Author

Reverted those changes.

@NewbieOrange NewbieOrange requested a review from remkop May 12, 2020 04:15
Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@remkop remkop merged commit bb892e9 into remkop:master May 12, 2020
@remkop remkop added this to the 4.3 milestone May 12, 2020
@remkop
Copy link
Owner

remkop commented May 12, 2020

Merged into master.
Thank you for the pull request!

remkop added a commit that referenced this pull request May 12, 2020
@NewbieOrange NewbieOrange deleted the code-cleanup branch May 12, 2020 07:53
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.

None yet

3 participants