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

Remove unnecessary return statements on lambdas and encapsulate public fields with fluent accessors #179

Conversation

bmscomp
Copy link
Member

@bmscomp bmscomp commented Oct 10, 2023

The purpose of this pull request is to bring some practices for reducing the number of lines on a growing class, and apply some practices, with some formatting to make it more readable and less verbose, notice that the maven formatter plugged into the project is used to ensure the conformity of the formatting

  • remove non necessary return statement on some lambdas
  • replace a check of instance of by non nullable check
  • remove duplicated cast for List<?>
  • encapsulated directly accessed public fields with fluent accessors

- remove non necessary return statement on some lambdas
- replace a check of instance of by non nullable check
- remove duplicated cast  for List<?>
@bmscomp bmscomp changed the title Remove unnecessary return statements on VaultPKIManager.java public methods Remove unnecessary return statements on lambdas for VaultPKIManager.java public methods Oct 10, 2023
@bmscomp bmscomp changed the title Remove unnecessary return statements on lambdas for VaultPKIManager.java public methods Remove unnecessary return statements on lambdas for VaultPKIManager.java Oct 10, 2023
@kdubb
Copy link
Contributor

kdubb commented Oct 10, 2023

IMO, many of these changes make the code less readable. There doesn't seem to be any advantage to replacing these single statement blocks with expressions, except to push the code 4-8 spaces to the right.

@bmscomp
Copy link
Member Author

bmscomp commented Oct 10, 2023

@kdubb thanks so much for the review, I agree the pull request is too minimal and it contains only a remove of the duplication and the nullability check instead of instance of but I can make it rich by updating it with other related topics, for examples the RoleOptions class that contains the contains all attributes as public and accessors that are not used, I can bring some encapsulation to it,

Please let me know what do you think about that ?

@bmscomp bmscomp changed the title Remove unnecessary return statements on lambdas for VaultPKIManager.java Remove unnecessary return statements on lambdas and encapsulate public fields with fluent accessors Oct 10, 2023
@bmscomp bmscomp force-pushed the code/remove_unecessary_return_statement branch from db0fe48 to e6e54e3 Compare October 10, 2023 20:55
@bmscomp
Copy link
Member Author

bmscomp commented Oct 10, 2023

@kdubb I updated the pull request to add a final keyword to a field in class and of course encapsulate fields SignIntermediateCAOptions.java and I used fluent accessors, as java suggest for records, to prevent non used methods

Copy link
Contributor

@kdubb kdubb left a comment

Choose a reason for hiding this comment

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

The biggest piece of this PR is source breaking and cannot be implemented without a major version bump. Additionally, the other "options" classes in the API are implemented similarly so we'd be changing just one of them, not all.

Finally, a point of personal preference (seemingly shared by @vsevel), there is no advantage to adding encapsulation to fields with public getters/setters for very simple classes like these "options" classes. Yes there can be advantages when the class/object will be long-lived or needs to interface with an API expecting a "bean". These objects are very transient and 99% of the time only live as long as a single function call.

I'm not sure this is something we need to tackle now or in the future.

WRT other changes. There are some documentation fixes that could be moved into another PR, which would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a public class. We can't make this change without breaking all existing code.

@kdubb
Copy link
Contributor

kdubb commented Mar 1, 2024

This is mostly implemented in #215 and the anyhting else is quite of out-of-date because of that PR.

@kdubb kdubb closed this Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants