refine string formatting in dependencyManagementKey #2483
Conversation
| nvl(dependency.getClassifier()), | ||
| nvl(dependency.getType())); | ||
| } | ||
| String hint = "groupId=" + nvl(dependency.getGroupId()) |
There was a problem hiding this comment.
This is even harder to read than what it replaced. I can't look at this code and figure out what it does. So three suggestions:
- Break it up with multiple local variables
- rename the nvl method
- Add detailed doc comment that specifies the format
There was a problem hiding this comment.
rework as you suggested. please tell me if this make it read easier.
| "groupId=%s, artifactId=%s, type=%s", | ||
| nvl(dependency.getGroupId()), nvl(dependency.getArtifactId()), nvl(dependency.getType())); | ||
| if (dependency.getClassifier() == null || dependency.getClassifier().isBlank()) { | ||
| hint = "groupId=" + valueToValueString(dependency.getGroupId()) |
There was a problem hiding this comment.
so this can really have groupId= rather than groupId="" ?
That seems strange
There was a problem hiding this comment.
so this can really have groupId= rather than groupId="" ?
according to the original codes, yes.
That seems strange
if you ask me for my ideas, yes, it is strange :)
| } | ||
| return new SourceHint(hint, null); // DMK | ||
| } | ||
|
|
||
| private static String nvl(String value) { | ||
| private static String valueToValueString(String value) { |
There was a problem hiding this comment.
Maybe just quote as a method name
Is it OK to have groupId=""? instead?
| @@ -2251,23 +2251,20 @@ public static SourceHint gav(String gav) { | |||
|
|
|||
| public static SourceHint dependencyManagementKey(Dependency dependency) { | |||
| hint = String.format( | ||
| "groupId=%s, artifactId=%s, type=%s", | ||
| nvl(dependency.getGroupId()), nvl(dependency.getArtifactId()), nvl(dependency.getType())); | ||
| if (dependency.getClassifier() == null || dependency.getClassifier().isBlank()) { |
There was a problem hiding this comment.
This might work better with a StringBuilder that adds a piece at a time. Then there'd be less duplicate code.
There was a problem hiding this comment.
This might work better with a StringBuilder that adds a piece at a time. Then there'd be less duplicate code.
yes, but it would make the codes harder to understand as you pointed out before...
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.