Issue 291680043: CurrencyDisplay SpacingInfo - Java (Closed)
|
Created:
7 years, 11 months ago by sffc Modified:
7 years, 8 months ago Reviewers:
markus.icu CC:
fabalbon Base URL:
svn+icussh://source.icu-project.org/repos/icu/icu4j/trunk/ Visibility:
Public. |
DescriptionTransitioning CurrencyDisplayInfo.getSpacingInfo to use data sinks.
Patch Set 1 #
Total comments: 16
Patch Set 2 : Updating with Markus's first round of feedback. #
Total comments: 2
Patch Set 3 : Updating with Markus's second round of feedback #
Total comments: 3
MessagesTotal messages: 13
https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... File main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:192: private final class SpacingInfoSink extends UResource.Sink { This seems verbose. Can you think of a design to make it more concise?
Sign in to reply to this message.
https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/impl/CurrencyData.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:3: * Copyright (C) 2009-2012, International Business Machines Corporation and * adjust to 2009-2016, and please remove the trailing (spaces star) from this line and next https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:51: if (strings.length != 6) { There is only one call site, right? I suggest you just do assert strings.length == 6; https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... File main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:235: int idx = pattern.ordinal() + type.ordinal() * SpacingPattern.values().length; Bad: SpacingPattern.values() creates a new array each time you call it (because arrays cannot be immutable). Please turn SpacingPattern.values().length into a private static final, right next to the definition of SpacingPattern. Optional: You could do this inside the enum SpacingPattern (maybe call it COUNT), then it would be used as SpacingPattern.COUNT which reads nicely. https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:236: spacingStrings[idx] = value.getString(); remember put-if-absent logic https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:243: for (String s : spacingStrings) { This is quite different from the original. The original code uses the loaded strings even if some or all are null, as long as there is both a beforeCurrency table and an afterCurrency table. I suggest you use two booleans (eg hasBeforeCurrency), set each for the corresponding type, and check here if both are set.
Sign in to reply to this message.
Updating with Markus's first round of feedback.
Sign in to reply to this message.
I made a few larger changes to CurrencySpacingInfo in order to make the pipeline more efficient and reduce copying between many different data structures. https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/impl/CurrencyData.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:3: * Copyright (C) 2009-2012, International Business Machines Corporation and * On 2016/05/13 17:29:08, markus.icu wrote: > adjust to 2009-2016, and please remove the trailing (spaces star) from this line > and next Done. https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:51: if (strings.length != 6) { On 2016/05/13 17:29:08, markus.icu wrote: > There is only one call site, right? > I suggest you just do > assert strings.length == 6; The one and only purpose of this class is to serve as an intermediate data format between loading the spacing patterns from the resource bundles, and saving them in their final home in DecimalFormatSymbols. Unfortunately, through that process, the data changes form several times: first its a String[6] in ICUCurrencyDisplayInfoProvider, then it's six private final strings in CurrencySpacingInfo, then it's two String[3]'s in DecimalFormatSymbols. I have re-written the class to make the data take its final form, two String[3]'s, directly in CurrencySpacingInfo. Let me know what you think. I can change it back if you don't like the idea. https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... File main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:235: int idx = pattern.ordinal() + type.ordinal() * SpacingPattern.values().length; On 2016/05/13 17:29:08, markus.icu wrote: > Bad: SpacingPattern.values() creates a new array each time you call it (because > arrays cannot be immutable). Please turn SpacingPattern.values().length into a > private static final, right next to the definition of SpacingPattern. > Optional: You could do this inside the enum SpacingPattern (maybe call it > COUNT), then it would be used as SpacingPattern.COUNT which reads nicely. Thanks. I like the COUNT option. https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:236: spacingStrings[idx] = value.getString(); On 2016/05/13 17:29:08, markus.icu wrote: > remember put-if-absent logic I'm not sure I understand? https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:243: for (String s : spacingStrings) { On 2016/05/13 17:29:08, markus.icu wrote: > This is quite different from the original. The original code uses the loaded > strings even if some or all are null, as long as there is both a beforeCurrency > table and an afterCurrency table. > > I suggest you use two booleans (eg hasBeforeCurrency), set each for the > corresponding type, and check here if both are set. Done.
Sign in to reply to this message.
https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/impl/CurrencyData.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:51: if (strings.length != 6) { On 2016/05/16 22:59:24, sffc wrote: > I have re-written the class to make the data take its final form, two > String[3]'s, directly in CurrencySpacingInfo. Let me know what you think. I > can change it back if you don't like the idea. I like it. I still suggest you turn the if+throw into an assert, given that it's an internal method with one call site. Ok if you don't want to make this change. https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... File main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:192: private final class SpacingInfoSink extends UResource.Sink { On 2016/05/10 01:33:27, sffc wrote: > This seems verbose. Can you think of a design to make it more concise? Sorry, can't think of a better way. https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:236: spacingStrings[idx] = value.getString(); On 2016/05/16 22:59:24, sffc wrote: > On 2016/05/13 17:29:08, markus.icu wrote: > > remember put-if-absent logic > > I'm not sure I understand? What I mean is, should you not check whether the value is already set before you set/overwrite it? Or is this supplemental data that has no parent-bundle inheritance? https://codereview.appspot.com/291680043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/impl/CurrencyData.java (right): https://codereview.appspot.com/291680043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:42: public static class CurrencySpacingInfo { Why did you remove "final"?
Sign in to reply to this message.
Updating with Markus's second round of feedback
Sign in to reply to this message.
Updating with Markus's second round of feedback
Sign in to reply to this message.
https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/impl/CurrencyData.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:51: if (strings.length != 6) { On 2016/05/28 19:26:14, markus.icu wrote: > On 2016/05/16 22:59:24, sffc wrote: > > I have re-written the class to make the data take its final form, two > > String[3]'s, directly in CurrencySpacingInfo. Let me know what you think. I > > can change it back if you don't like the idea. > > I like it. > > I still suggest you turn the if+throw into an assert, given that it's an > internal method with one call site. Ok if you don't want to make this change. Done. https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... File main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java (right): https://codereview.appspot.com/291680043/diff/1/main/classes/currdata/src/com... main/classes/currdata/src/com/ibm/icu/impl/ICUCurrencyDisplayInfoProvider.java:236: spacingStrings[idx] = value.getString(); On 2016/05/28 19:26:14, markus.icu wrote: > On 2016/05/16 22:59:24, sffc wrote: > > On 2016/05/13 17:29:08, markus.icu wrote: > > > remember put-if-absent logic > > > > I'm not sure I understand? > > What I mean is, should you not check whether the value is already set before you > set/overwrite it? Or is this supplemental data that has no parent-bundle > inheritance? Yes, right. Fixed. It doesn't appear that this particular item has data anywhere other than root.txt, but it's good to be future-proof. https://codereview.appspot.com/291680043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/impl/CurrencyData.java (right): https://codereview.appspot.com/291680043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/impl/CurrencyData.java:42: public static class CurrencySpacingInfo { On 2016/05/28 19:26:15, markus.icu wrote: > Why did you remove "final"? Don't know. Changed it back.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
On 2016/05/30 18:13:38, markus.icu wrote: > LGTM Submitted as r38772.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/291680043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java (right): https://codereview.appspot.com/291680043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:1072: currencySpcBeforeSym = spcInfo.getBeforeSymbols(); Coverity pointed out that it is possible for spcInfo to be null. This would happen if there is no data available and the user specified no fallback. What would be the best way to handle such a case?
Sign in to reply to this message.
https://codereview.appspot.com/291680043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java (right): https://codereview.appspot.com/291680043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:1072: currencySpcBeforeSym = spcInfo.getBeforeSymbols(); On 2016/08/10 02:37:35, sffc wrote: > Coverity pointed out that it is possible for spcInfo to be null. This would > happen if there is no data available and the user specified no fallback. What > would be the best way to handle such a case? We should probably fall back to hardcoded data if all else fails. How did the pre-enumeration code handle this case?
Sign in to reply to this message.
https://codereview.appspot.com/291680043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java (right): https://codereview.appspot.com/291680043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:1072: currencySpcBeforeSym = spcInfo.getBeforeSymbols(); On 2016/08/11 22:30:47, markus.icu wrote: > On 2016/08/10 02:37:35, sffc wrote: > > Coverity pointed out that it is possible for spcInfo to be null. This would > > happen if there is no data available and the user specified no fallback. What > > would be the best way to handle such a case? > > We should probably fall back to hardcoded data if all else fails. How did the > pre-enumeration code handle this case? There are indeed hard-coded fallbacks, but they can be disabled by an argument to CurrencyData.getInstance(). However, it appears that DecimalFormatSymbols always accepts the fallback strings: CurrencyDisplayInfo info = CurrencyData.provider.getInstance(locale, true); where "true" is the optional fallback parameter. So spcInfo can never be null. I'll dismiss the error in Coverity.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
