Remove dcc common and dcc id dependency#503 #574
Conversation
rtisma
left a comment
rtisma
left a comment
There was a problem hiding this comment.
As you can see, i am allergic to string literals. If the same literal gets repeated more than once, it should instead be referenced as a variable. I left a few comments for the Splitters and Joiners class that needs to be made, but there are several other lines that need similar fixing and i didnt comment on them so that this review isnt riddled with identical comments.
| <dependency> | ||
| <groupId>org.glassfish.jaxb</groupId> | ||
| <artifactId>jaxb-runtime</artifactId> | ||
| <version>2.3.1</version> |
There was a problem hiding this comment.
move this version definition into the <dependencyManagement> block of the root pom.xml. If this dependency is only used in song-server, then rip out the dependency from the dependencyManagement section and put it here. Also, use the version variable like ${jaxb.version}
There was a problem hiding this comment.
String constants are a valid part of the Java language. Don't be so afraid of them!
If we expect the definition of a constant to ever change, then YES, by all means, we should use a name rather than an inline value.
OBJECT_NAME_DELIMITER might change. NEWLINE might change (based upon different operating systems idea of what a new line should be).
But the definition of a something like COMMA should never change, and if it does, it's confusing.
All we accomplish by separating the definition of the constant from the code in this case is forcing the developer to do extra work to double check that COMMA is defined as ",".
There was a problem hiding this comment.
i find that string constants are harder to read and make it hard to search for via the IDE. by using COMMA, you can inspect its usage easily. Regarding double checking, for something that sensitive, there should be tests in place that ensure it functions as expected.
There was a problem hiding this comment.
I'm confused. How is searching for "," any harder than searching for COMMA? If anything, it should be easier, since it's two less characters to type. :-)
There should always be tests. That doesn't mean that the code does what you think it does when you read it.:-)
But for consistency, I'll make you happy. :-)
| @RequestParam(value = "analysisStates", defaultValue = "PUBLISHED", required = false) | ||
| String analysisStates) { | ||
| return analysisService.getAnalysis(studyId, ImmutableSet.copyOf(COMMA.split(analysisStates))); | ||
| return analysisService.getAnalysis(studyId, ImmutableSet.copyOf(",".split(analysisStates))); |
There was a problem hiding this comment.
Create a Splitters class with a no args constructor, that contains:
public static final Splitter COMMA = Splitter.on(",");
and likewise for any other splits.
| public static final Direction DEFAULT_SORT_ORDER = DESC; | ||
|
|
||
| private static final String ALLOWED_SORT_VARIABLES = COMMA.join(VERSION, NAME); | ||
| private static final String ALLOWED_SORT_VARIABLES = VERSION + "," + NAME; |
There was a problem hiding this comment.
Create a no args constuctor Joiners class that contains the following:
public static final Joiner COMMA = Joiner.on(",");
likewise for any other characters to join on
|
|
||
| private static List<String> getFullStackTraceList(Throwable t) { | ||
| return NEWLINE.splitToList(getStackTraceAsString(t)).stream() | ||
| return stream(getStackTraceAsString(t).split("\n")) |
| + "analysisId %s can be published: %s", | ||
| analysisId, | ||
| COMMA.join(missingFileIds)); | ||
| join(",", missingFileIds)); |
rtisma
left a comment
rtisma
left a comment
There was a problem hiding this comment.
also, there are conflicts with the branch
… for commonly used string constants. Change: Now use the Separators enum and avoid the use of String constants.
| import java.util.List; | ||
|
|
||
| /** Joiners and Splitters for commonly used separators. */ | ||
| public enum Separators { |
Now compiles, builds, and passes make test-*.