Issue 290110043: Initial version of DateIntervalFormat data loading using the Sink (Closed)
|
Created:
8 years, 2 months ago by fabalbon Modified:
8 years, 1 month ago Reviewers:
markus.icu Base URL:
svn+ssh://source.icu-project.org/repos/icu/icu4j/branches/fabalbon/12013/ Visibility:
Public. |
DescriptionInitial version of DateIntervalFormat data loading using the Sink
Patch Set 1 #
Total comments: 46
Patch Set 2 : Code review changes r38304 #
Total comments: 20
Patch Set 3 : Code review changes r38306 #
Total comments: 2
Patch Set 4 : Code review changes r38306 #Patch Set 5 : Code review changes r38322 #
Total comments: 6
Patch Set 6 : Code review changes r38339 #
Total comments: 1
MessagesTotal messages: 20
https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:3: * Copyright (C) 2008-2015, International Business Machines Corporation and * Update to 2008-2016. While you are at it, please remove the trailing spaces+asterisk on this line and the next line. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:410: On lines that you add or edit, please make sure you don't have trailing spaces. Please set Eclipse to display trailing whitespace. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:421: class DateIntervalSkeletonSink extends UResource.TableSink { On nested classes, remove the "DateInterval" prefix from the class names. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:426: class DateIntervalPatternSink extends UResource.TableSink { Please don't nest these classes multiple times -- just one level inside the outer sink. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:429: HashSet<String> skeletonPatternLetterPairs = new HashSet<String>(); It is customary in Java to declare container variables more generically, like Set<String>, and only be specific in the "new". https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:432: //Instance methods: Not useful, remove. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:435: * {@inheritDoc} Not useful, remove. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:439: //TODO(fabalbon): check for nulls in key and value?? key and value will never be null https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:442: //If the calendar field has a valid value Please put a space after the // https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:444: //TODO(fabalbon): What to do with the debug skeleton? (refer to the original method) ok to drop https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:448: String intervalPattern = value.toString(); Defer this until you know you need it. We don't want to deserialize strings that we ignore. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:455: dateIntervalInfo.setIntervalPatternInternally(currentSkeleton, lrgDiffCalUnit, intervalPattern); Please try to keep line lengths to roughly 100. A little more is ok if it's tedious to wrap. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:510: //TODO(fabalbon): should I check if the skeleton is valid?? not unless the old code did so https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:538: if (key.toString().compareTo(DateIntervalInfo.INTERVAL_FORMATS_KEY) != 0 Please check the value.getType() first, and then use key.contentEquals(DateIntervalInfo.INTERVAL_FORMATS_KEY) to avoid deserializing the key into a String. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:545: if (aliasPath == null) { return; } never https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:548: String calendarType = getCalendarTypeFromPath(aliasPath); Just nextCalendarType = getCalendarTypeFromPath(value.getAliasString()); should do. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:560: if (key.toString().compareTo(DateIntervalInfo.INTERVAL_FORMATS_KEY) == 0) { key.contentEquals() https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:570: public String getNextCalendarType() { No need for a getter, just have the caller access the field directly. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:575: * Resets the next calendar type. The method name speaks for itself. I would just have the caller reset the field directly, but you could keep this method if you like. In that case, I would change it to "String getAndResetNextCalendarType()". https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:587: //TODO(fabalbon): Isn't there an existing method I could use for this? if (path.startsWith(everything before the type) && path.endsWith(everything after the type)) { return path.substring(fixed length of the prefix, path.length() - fixed length of the suffix); } Easiest if you make private static final String constants for the prefix & suffix; you can use their .length()s instead of counting yourself. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:626: //Instance the sink to process the data and the resource bundle As a verb: Instance -> Instantiate https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:638: while (calendarTypeToUse != null && !loadedCalendarTypes.contains(calendarTypeToUse)) { Rather than checking for a calendar-type loop here, please add the following right inside the while: if (loadedCalendarTypes.contains(calendarTypeToUse)) { throw new ICUException("Loop in calendar type fallback: " + calendarTypeToUse); }
Sign in to reply to this message.
Code review changes r38304
Sign in to reply to this message.
Code review changes https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:3: * Copyright (C) 2008-2015, International Business Machines Corporation and * On 2016/02/10 00:43:39, markus.icu wrote: > Update to 2008-2016. While you are at it, please remove the trailing > spaces+asterisk on this line and the next line. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:410: On 2016/02/10 00:43:39, markus.icu wrote: > On lines that you add or edit, please make sure you don't have trailing spaces. > > Please set Eclipse to display trailing whitespace. I'll do this on a follow up commit so that the diffs are clearer. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:421: class DateIntervalSkeletonSink extends UResource.TableSink { On 2016/02/10 00:43:39, markus.icu wrote: > On nested classes, remove the "DateInterval" prefix from the class names. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:426: class DateIntervalPatternSink extends UResource.TableSink { On 2016/02/10 00:43:39, markus.icu wrote: > Please don't nest these classes multiple times -- just one level inside the > outer sink. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:429: HashSet<String> skeletonPatternLetterPairs = new HashSet<String>(); On 2016/02/10 00:43:39, markus.icu wrote: > It is customary in Java to declare container variables more generically, like > Set<String>, and only be specific in the "new". Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:432: //Instance methods: On 2016/02/10 00:43:39, markus.icu wrote: > Not useful, remove. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:435: * {@inheritDoc} On 2016/02/10 00:43:39, markus.icu wrote: > Not useful, remove. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:439: //TODO(fabalbon): check for nulls in key and value?? On 2016/02/10 00:43:40, markus.icu wrote: > key and value will never be null Acknowledged. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:442: //If the calendar field has a valid value On 2016/02/10 00:43:39, markus.icu wrote: > Please put a space after the // Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:444: //TODO(fabalbon): What to do with the debug skeleton? (refer to the original method) On 2016/02/10 00:43:40, markus.icu wrote: > ok to drop Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:455: dateIntervalInfo.setIntervalPatternInternally(currentSkeleton, lrgDiffCalUnit, intervalPattern); On 2016/02/10 00:43:39, markus.icu wrote: > Please try to keep line lengths to roughly 100. A little more is ok if it's > tedious to wrap. Acknowledged. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:545: if (aliasPath == null) { return; } On 2016/02/10 00:43:40, markus.icu wrote: > never Acknowledged. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:548: String calendarType = getCalendarTypeFromPath(aliasPath); On 2016/02/10 00:43:40, markus.icu wrote: > Just > nextCalendarType = getCalendarTypeFromPath(value.getAliasString()); > should do. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:560: if (key.toString().compareTo(DateIntervalInfo.INTERVAL_FORMATS_KEY) == 0) { On 2016/02/10 00:43:39, markus.icu wrote: > key.contentEquals() Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:570: public String getNextCalendarType() { On 2016/02/10 00:43:39, markus.icu wrote: > No need for a getter, just have the caller access the field directly. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:575: * Resets the next calendar type. On 2016/02/10 00:43:39, markus.icu wrote: > The method name speaks for itself. > > I would just have the caller reset the field directly, but you could keep this > method if you like. In that case, I would change it to "String > getAndResetNextCalendarType()". Acknowledged. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:587: //TODO(fabalbon): Isn't there an existing method I could use for this? On 2016/02/10 00:43:39, markus.icu wrote: > if (path.startsWith(everything before the type) && path.endsWith(everything > after the type)) { > return path.substring(fixed length of the prefix, path.length() - fixed > length of the suffix); > } > > Easiest if you make private static final String constants for the prefix & > suffix; you can use their .length()s instead of counting yourself. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:626: //Instance the sink to process the data and the resource bundle On 2016/02/10 00:43:39, markus.icu wrote: > As a verb: Instance -> Instantiate Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:638: while (calendarTypeToUse != null && !loadedCalendarTypes.contains(calendarTypeToUse)) { On 2016/02/10 00:43:40, markus.icu wrote: > Rather than checking for a calendar-type loop here, please add the following > right inside the while: > > if (loadedCalendarTypes.contains(calendarTypeToUse)) { > throw new ICUException("Loop in calendar type fallback: " + > calendarTypeToUse); > } Done.
Sign in to reply to this message.
https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:465: private int patternLetterToCalendarField(String patternLetter) { I would like to optimize this method as we discussed the other day, is this a good moment to do it? O should I wait until the first version is submitted?
Sign in to reply to this message.
https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:548: String calendarType = getCalendarTypeFromPath(aliasPath); On 2016/02/11 22:14:19, fabalbon wrote: > On 2016/02/10 00:43:40, markus.icu wrote: > > Just > > nextCalendarType = getCalendarTypeFromPath(value.getAliasString()); > > should do. > > Done. No, you are still checking for null. I think you don't need to. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:429: SkeletonSink dateIntervalSkeletonSink = new SkeletonSink(); Please remove the "dateInterval" prefix from these variable names too. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:444: if (calendarField != -1) { I prefer this kind of test as if (calendarField >= 0) { https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:453: dateIntervalInfo.setIntervalPatternInternally(currentSkeleton, lrgDiffCalUnit, Does it make sense to turn this into a setIntervalPatternIfAbsent(...) (taking a UResource.Value as the last param to defer the .toString()), have that method check for already-there, and remove the skeletonPatternLetterPairs Set? I don't remember if there was another reason for having the Set. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:465: private int patternLetterToCalendarField(String patternLetter) { You can optimize this now. Please change the parameter to a CharSequence to avoid key.toString(). Bail out if .length() != 1, otherwise look at .charAt(0). https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:503: public String nextCalendarType; FYI: In Java, inner & outer classes have access even to each others' private fields, so no need to make stuff public here. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:517: if (!key.contentEquals(INTERVAL_FORMATS_KEY) For efficiency, please check first for the type==alias, then for the key. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:561: path.endsWith(DATE_INTERVAL_PATH_SUFIX)) { indent 4 more (like in google3 style), otherwise it's hard to distinguish the wrapped condition from the body https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:607: // Throw a exception when a loop is detected a -> an
Sign in to reply to this message.
Code review changes r38305
Sign in to reply to this message.
Code review changes r38306
Sign in to reply to this message.
Second code review changes https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:448: String intervalPattern = value.toString(); On 2016/02/10 00:43:39, markus.icu wrote: > Defer this until you know you need it. We don't want to deserialize strings that > we ignore. Done. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:510: //TODO(fabalbon): should I check if the skeleton is valid?? On 2016/02/10 00:43:39, markus.icu wrote: > not unless the old code did so Acknowledged. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:538: if (key.toString().compareTo(DateIntervalInfo.INTERVAL_FORMATS_KEY) != 0 On 2016/02/10 00:43:40, markus.icu wrote: > Please check the value.getType() first, and then use > key.contentEquals(DateIntervalInfo.INTERVAL_FORMATS_KEY) to avoid deserializing > the key into a String. Acknowledged. https://codereview.appspot.com/290110043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:548: String calendarType = getCalendarTypeFromPath(aliasPath); On 2016/02/12 00:42:43, markus.icu wrote: > On 2016/02/11 22:14:19, fabalbon wrote: > > On 2016/02/10 00:43:40, markus.icu wrote: > > > Just > > > nextCalendarType = getCalendarTypeFromPath(value.getAliasString()); > > > should do. > > > > Done. > > No, you are still checking for null. I think you don't need to. Yes, that's because getCalendarTypeFromPath returns null if the intervalFormats alias is not like: "/LOCALE/calendar/<type>/intervalFormats". Maybe I could modify getCalendarTypeFromPath to throw an ICUException instead of returning null because if this happens it's an error and not an expected case. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:429: SkeletonSink dateIntervalSkeletonSink = new SkeletonSink(); On 2016/02/12 00:42:44, markus.icu wrote: > Please remove the "dateInterval" prefix from these variable names too. Done. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:444: if (calendarField != -1) { On 2016/02/12 00:42:43, markus.icu wrote: > I prefer this kind of test as > > if (calendarField >= 0) { The whole method was changed. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:453: dateIntervalInfo.setIntervalPatternInternally(currentSkeleton, lrgDiffCalUnit, On 2016/02/12 00:42:43, markus.icu wrote: > Does it make sense to turn this into a setIntervalPatternIfAbsent(...) (taking a > UResource.Value as the last param to defer the .toString()), have that method > check for already-there, and remove the skeletonPatternLetterPairs Set? I don't > remember if there was another reason for having the Set. Yes I agree, we could eliminate skeletonPatternLetterPairs. The skeletonPatternLetterPairs set is used only to check if the format has been added, and I don't think there is a big performance difference compared to checking directly in the internal data structure (1 step Set.contains vs 2 step nested Map.contains) Regarding the implementation, I think I prefer to have setIntervalPatternIfAbsent inside the sink instead of in DateIntervalInfo together with the other setIntervalPattern methods. This is because I feel that receiving a UResource.Value as a parameter to defer the deserelization is too specific to the sink to have it directly on DateIntervalInfo. What do you think? https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:465: private int patternLetterToCalendarField(String patternLetter) { On 2016/02/12 00:42:43, markus.icu wrote: > You can optimize this now. Please change the parameter to a CharSequence to > avoid key.toString(). Bail out if .length() != 1, otherwise look at .charAt(0). Done. I would like to change CALENDAR_FIELD_TO_PATTERN_LETTER from a String[] with a one letter string for each pattern letter to a single String and charAt() as we discussed the other day. What do you think? This will require to refactor a couple of lines in DateIntervalFormat.java as well. I'd encapsulate substring and charAt in helper methods. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:503: public String nextCalendarType; On 2016/02/12 00:42:43, markus.icu wrote: > FYI: In Java, inner & outer classes have access even to each others' private > fields, so no need to make stuff public here. Acknowledged. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:517: if (!key.contentEquals(INTERVAL_FORMATS_KEY) On 2016/02/12 00:42:44, markus.icu wrote: > For efficiency, please check first for the type==alias, then for the key. Done. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:561: path.endsWith(DATE_INTERVAL_PATH_SUFIX)) { On 2016/02/12 00:42:43, markus.icu wrote: > indent 4 more (like in google3 style), otherwise it's hard to distinguish the > wrapped condition from the body Acknowledged. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:607: // Throw a exception when a loop is detected On 2016/02/12 00:42:43, markus.icu wrote: > a -> an Done.
Sign in to reply to this message.
Something seems to have gone wrong with uploading the patch. When I ask for diffs from base, it shows diffs from the previous round, not from the actual base. This Java code is getting close, good job. Note that I will be on vacation next week. You could ask Andy to help you get started on C++ in the meantime. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:453: dateIntervalInfo.setIntervalPatternInternally(currentSkeleton, lrgDiffCalUnit, On 2016/02/12 20:32:51, fabalbon wrote: > On 2016/02/12 00:42:43, markus.icu wrote: > > Does it make sense to turn this into a setIntervalPatternIfAbsent(...) (taking > a > > UResource.Value as the last param to defer the .toString()), have that method > > check for already-there, and remove the skeletonPatternLetterPairs Set? I > don't > > remember if there was another reason for having the Set. > > Yes I agree, we could eliminate skeletonPatternLetterPairs. > > The skeletonPatternLetterPairs set is used only to check if the format has been > added, and I don't think there is a big performance difference compared to > checking directly in the internal data structure (1 step Set.contains vs 2 step > nested Map.contains) > > Regarding the implementation, I think I prefer to have > setIntervalPatternIfAbsent inside the sink instead of in DateIntervalInfo > together with the other setIntervalPattern methods. This is because I feel that > receiving a UResource.Value as a parameter to defer the deserelization is too > specific to the sink to have it directly on DateIntervalInfo. What do you think? Acknowledged. https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:465: private int patternLetterToCalendarField(String patternLetter) { On 2016/02/12 20:32:51, fabalbon wrote: > I would like to change CALENDAR_FIELD_TO_PATTERN_LETTER from a String[] with a > one letter string for each pattern letter to a single String and charAt() as we > discussed the other day. What do you think? This will require to refactor a > couple of lines in DateIntervalFormat.java as well. I'd encapsulate substring > and charAt in helper methods. A single String would be much better. Refactoring other code is fine, as long as it's internal. I don't like "Set<Character> ACCEPTED_PATTERN_LETTERS". Just use a String and test for s.indexOf(char letter) >= 0. https://codereview.appspot.com/290110043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:458: if (patternLetter.length() != 1 || !ACCEPTED_PATTERN_LETTERS.contains(patternLetter.charAt(0))) { Please call charAt(0) only once and use a char variable for it. How about if (patternLetter.length() != 1) { return null; } char letter = patternLetter.charAt(0); if (!ACCEPTED_PATTERN_LETTERS.contains(letter)) { return null; } ... use letter again ...
Sign in to reply to this message.
Code review changes r38306
Sign in to reply to this message.
HI Markus, I Uploaded a new patch, I think this one got uploaded correctly. Now I'll start working on your new comments! Yes, I'll ask andy for help to start working on the C++ version. Thanks for your thorough review!
Sign in to reply to this message.
Code review changes r38322
Sign in to reply to this message.
Third code review changes. Thanks!! https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:465: private int patternLetterToCalendarField(String patternLetter) { On 2016/02/12 23:19:51, markus.icu wrote: > On 2016/02/12 20:32:51, fabalbon wrote: > > I would like to change CALENDAR_FIELD_TO_PATTERN_LETTER from a String[] with a > > one letter string for each pattern letter to a single String and charAt() as > we > > discussed the other day. What do you think? This will require to refactor a > > couple of lines in DateIntervalFormat.java as well. I'd encapsulate substring > > and charAt in helper methods. > > A single String would be much better. Refactoring other code is fine, as long as > it's internal. > > I don't like "Set<Character> ACCEPTED_PATTERN_LETTERS". > Just use a String and test for s.indexOf(char letter) >= 0. Done. https://codereview.appspot.com/290110043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:458: if (patternLetter.length() != 1 || !ACCEPTED_PATTERN_LETTERS.contains(patternLetter.charAt(0))) { On 2016/02/12 23:19:51, markus.icu wrote: > Please call charAt(0) only once and use a char variable for it. How about > if (patternLetter.length() != 1) { > return null; > } > char letter = patternLetter.charAt(0); > if (!ACCEPTED_PATTERN_LETTERS.contains(letter)) { > return null; > } > ... use letter again ... Done.
Sign in to reply to this message.
https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:407: public static String calendarFieldToPatternLetter(int calendarField) Not public -- we don't want to make it user-visible API. Just remove "public" to make it package-visible, like the old array used to be. *But* see the next comment. https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:409: return CALENDAR_FIELD_TO_PATTERN_LETTER.substring(calendarField, calendarField + 1); This is actually quite inefficient. It creates a new String object in each call, and copies the one-character contents. It looks like it might be best to revert this to the old array. Using a single String for ACCEPTED_PATTERN_LETTERS works nicely, but it does not seem to make sense for these strings. https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:500: dateIntervalInfo.fIntervalPatterns.get(currentSkeleton); Either use the skeleton argument, or remove that argument and keep using the currentSkeleton. Probably the latter (use currentSkeleton).
Sign in to reply to this message.
Code review changes r38339
Sign in to reply to this message.
Code review changes r38339
Sign in to reply to this message.
Code review changes r38339
Sign in to reply to this message.
Hi Markus, If there are not further comments on the next commit I'll remove all trailing whitespaces. Thanks! https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:407: public static String calendarFieldToPatternLetter(int calendarField) On 2016/02/23 19:49:16, markus.icu wrote: > Not public -- we don't want to make it user-visible API. > Just remove "public" to make it package-visible, like the old array used to be. > > *But* see the next comment. Done. https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:409: return CALENDAR_FIELD_TO_PATTERN_LETTER.substring(calendarField, calendarField + 1); On 2016/02/23 19:49:16, markus.icu wrote: > This is actually quite inefficient. It creates a new String object in each call, > and copies the one-character contents. It looks like it might be best to revert > this to the old array. > > Using a single String for ACCEPTED_PATTERN_LETTERS works nicely, but it does not > seem to make sense for these strings. Done. https://codereview.appspot.com/290110043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:500: dateIntervalInfo.fIntervalPatterns.get(currentSkeleton); On 2016/02/23 19:49:16, markus.icu wrote: > Either use the skeleton argument, or remove that argument and keep using the > currentSkeleton. Probably the latter (use currentSkeleton). Done.
Sign in to reply to this message.
One nit. Otherwise, ok to remove trailing spaces. Then just check in and merge into the trunk. https://codereview.appspot.com/290110043/diff/160001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/290110043/diff/160001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:630: } } catch (MissingResourceException e) {
Sign in to reply to this message.
|
