Prohibit String.replace() and String.replaceAll(), fix and prohibit some toString()-related redundancies#6607
Conversation
…ome toString()-related redundancies
leventov
force-pushed
the
implicit-pattern
branch
from
16c26a6 to
4371004
Compare
| private static String removeChar(String s, char c, int firstOccurranceIndex) | ||
| /** | ||
| * Removes all occurrences of the given char from the given string. This method is an optimal version of | ||
| * {@link String#replace(CharSequence, CharSequence) s.replace("c", "")}. |
There was a problem hiding this comment.
i think the description is not accurate, what is "" in terms of char ?
There was a problem hiding this comment.
It's an empty string. s.replace(":", "") is literally the pattern that should be replaced with removeChar(s, ':') in the Druid code.
| * Replaces all occurrences of the given target substring in the given string with the given replacement string. This | ||
| * method is an optimal version of {@link String#replace(CharSequence, CharSequence) s.replace(target, replacement)}. | ||
| */ | ||
| public static String replace(String s, String target, String replacement) |
There was a problem hiding this comment.
although the original method String#replace(CharSequence, CharSequence) has CharSequence as API, the one you are proposing is using String (String s, String target, String replacement) wondering if that is totally equivalent as a replacement since it is only covering a subset of it?
There was a problem hiding this comment.
There are uses for it yet. BTW even the new OpenJDK 9+ version converts the target to String internally because the implementation relies on indexOf() that is optimized only for two Strings, i. e. making the target a CharSequence will only give false sense of "optimality". The replacement could be made CharSequence but there is no need for that yet.
…rage (#6510) * 1. added support for unused DateTime start parameter in getRecentlyFinishedTaskInfoSince method: HeapMemoryTaskStorage.getRecentlyFinishedTaskInfoSince return the finished tasks by comparing TaskStuff.createdDate with the start time 2. added filtering by status complete to TaskStuff list stream in HeapMemoryTaskStorage.getNRecentlyFinishedTaskInfo method. 3. changed names of methods and parameters to present that public API method OverlordResource.getTasks return the list of completed tasks, which createdDate, not date of completion, belongs to the interval parameter. * 1. added support for unused DateTime start parameter in getRecentlyFinishedTaskInfoSince method: HeapMemoryTaskStorage.getRecentlyFinishedTaskInfoSince return the finished tasks by comparing TaskStuff.createdDate with the start time 2. added filtering by status complete to TaskStuff list stream in HeapMemoryTaskStorage.getNRecentlyFinishedTaskInfo method. 3. changed names of methods and parameters to present that public API method OverlordResource.getTasks return the list of completed tasks, which createdDate, not date of completion, belongs to the interval parameter. * Fixed OverlordResourceTest to Support changed methods names * Changed methods and parameters names to make them more obvious to understand. * Changed String.replace() for the StringUtils.replace()(#6607) * Fixed checkstyle error
4 participants
This PR primarily prohibits methods in
Stringclass that compile aPatternimplicitly:String.matches()String.replace()(It should have not in the first place; it's fixed in OpenJDK 9+)String.replaceAll()String.replaceFirst()I've seen 10% of allocations on a Coordinator instance to happen in
Pattern.compile()in one of thoseString.replace()calls.Also prohibited and fixed some other String-related redundancies and inefficient patterns.