Skip to content

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented May 17, 2021

No description provided.

private final String namespace;

private Map<String, String> labels = new HashMap<>();
private final Map<String, String> labels;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is redundant, as the only way to set labels is via the constructor.


public SecretsConfigProperties.NormalizedSource normalize(String defaultName, String defaultNamespace,
Map<String, String> defaultLabels) {
final String normalizedName = StringUtils.isEmpty(this.name) ? defaultName : this.name;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isEmpty is deprecated, replace it with hasLength. Also final is not really needed

return new ArrayList<SecretsConfigProperties.NormalizedSource>() {
{
add(new SecretsConfigProperties.NormalizedSource(SecretsConfigProperties.this.name,
return Collections
Copy link
Contributor Author

Choose a reason for hiding this comment

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

double-brace initialization is a common anti-pattern, replace with a singletonList


@Override
public String getConfigurationTarget() {
return TARGET;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unless a String is repeated in code for readability reasons, it is not needed as a constant in the form of private static final

@wind57 wind57 marked this pull request as ready for review May 18, 2021 14:27
@ryanjbaxter ryanjbaxter added this to the 2.0.3 milestone May 18, 2021
@ryanjbaxter ryanjbaxter merged commit f43b5c1 into spring-cloud:main May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants