On Thu, Oct 8, 2009 at 5:16 PM, Mark Hindess
<mark.hind...@googlemail.com> wrote:
>
> In message <4ace0d32.3060...@googlemail.com>, Oliver Deakin writes:
>>
>> Tim Ellison wrote:
>> > On 08/Oct/2009 14:21, Oliver Deakin wrote:
>> >
>> >> Tim Ellison wrote:
>> >>
>> >>> Part of the patch for HARMONY-6346 includes some improvements to the way
>> >>> we handle localized exception messages. That prompted me to write down
>> >>> a few thoughts...
>> >>>
>> >>> I don't really like the way we do I18N of exception messages. We create
>> >>> message string like this
>> >>>
>> >>> throw new IOException(Messages.getString("archive.1E"));
>> >>>
>> >>>
>> >> One way to handle this could be to use an exception manager class.
>> >> Rather than carrying out the lookup on the error message at the time the
>> >> exception is thrown, instead associate the exception object with the
>> >> error message key using a exception manager class. Then in the
>> >> getMessage(), getLocalizedMessage() etc. methods the exception will call
>> >> into the exception manager to get it's message (and can cache it locally
>> >> in case of future lookups) if one exists.
>> >>
>> >> So instead of:
>> >> throw new IOException(Messages.getString("archive.1E"));
>> >>
>> >> you might have (here create() returns the exception passed to it as the
>> >> first parameter):
>> >> throw ExceptionManager.create(new IOException(), "archive.1E");
>
> or:
>
> ExceptionManager.throw(new IOException(), "archive.1E")
>
> and let the Exception manager throw it? Or maybe that would "ruin"
> the stack trace.
It would ruin the stack trace, we want to leave the throw where it is.
>
>> >> Once the exception is caught and it's message string queried, it will
>> >> call ExceptionManager.getMessage(this) to see if it has an associated
>> >> message available. This way the error message lookup will only happen if
>> >> the message is actually queried. The references in the ExceptionManager
>> >> could be soft so that if the exception is not caught or is discarded,
>> >> then the ExceptionManager does not unnecesarily hang onto it's error
>> >> message.
>> >>
>> >
>> > Yep, so to replay what you just said, the 'guts' of the ExceptionManager
>> > would be a WeakHashMap<Throwable, Object[]> of 'pending' exceptions that
>> > may be asked for their formatted message.
>> >
>> > The Object[] would contain the message id and arguments that only get
>> > looked up and formatted if asked.
>> >
>>
>> Sounds about right to me.
>>
>> >
>> >>> The string "archive.1E" is a key into the resource files loaded by the
>> >>> Messages class based on the current locale.
>> >>>
>> >>> 1) I have no concrete proof, but I'm prepared to bet that 90% of the
>> >>> exceptions thrown never get asked for their message string. Yet we are
>> >>> setting it eagerly - mainly because there is no mechanism on exceptions
>> >>> to answer the message lazily.
>> >>>
>> >>> Simple messages may not be too bad, but we also have formatted messages
>> >>> where we replace token in the message with actual parameters. A
>> >>> particular bad example requires formatting more argument strings, e.g.
>> >>>
>> >>> throw new IllegalArgumentException(
>> >>> Msg.getString("K031e", new String[] { "query", uri.toString() }));
>> >>>
>> >>> in this case we format the URI and format the message string, and
>> >>> probably never use the answer.
>> >>>
>> >>>
>> >> For this example, the create() method could be made to accept variable
>> >> arguments. It's first argument would be the exception object, the second
>> >> would be the message identifier, and any subsequent arguments would be
>> >> the values to be inserted into tokens in the message string. This would
>> >> mean that in the above example the uri.toString() would still be
>> >> evaluated at the time the exception was created, but the lookup and
>> >> formatting of the error string would only be done when the actual
>> >> exception message was queried. (I havn't looked deeply at this case so
>> >> it may be more complex then that).
>> >>
>> >
>> > ...and if you wanted to be sneaky, you could change the message id to be
>> > the actual string for `you favorite locale` which would mean there was
>> > no loading of message resource bundles if you happened to be running in
>> > the most-favored-locale, e.g.
>> >
>> > throw ExceptionManager.create(new IOException(), "Stream is closed");
>> >
>>
>> True, although I'm not entirely convinced this is a good idea for a
>> couple of reasons:
>> - Changing all our message files to have "Full English message"="Full
>> translated message" might make them quite cluttered and hard to read.
>
> You read those? You should get out more. ;-)
>
> Personally I read the code more often and having a meaningful string
> instead of a token definitely makes it easier. I also think it would
> make mistakes less likely. Consider my commit r822652 yesterday for
> HARMONY-6347 compared with the original patch. The original patch
> changed:
>
> if (len < 0)
> throw new ArrayIndexOutOfBoundsException(Messages.getString("crypto.36"));
>
> to:
>
> if (len < 0 || offset < 0)
> throw new ArrayIndexOutOfBoundsException(Messages.getString("crypto.36"));
>
> which would have given the message "len is negative" for either case. I
> think this kind of mistake[0] would be less likely with the English text
> present.
I can live without the actual English message, if the resource keys
were semantic - "length_less_than_zero" instead of "crypto.36". I
never thought it saved much to use a hex numbering scheme.
Any possibility to just doing something much more simple, like a
package-scope class named ExceptionMessages with something simple like
String[] constants with semantic names that are offset by locale. If
loading a properties file is expensive, is it any less expensive to
put it in a class file?
>
>> - Some messages may contain the '=' character (for example, archive.15)
>> which would mess up the parsing of message files.
>
> I'm fairly sure having the same format would not be sensible but that is
> entirely under our control (and Tim already mentioned that Properties/
> ResourceBundles are rather expensive for a lookup mechanism). However,
> I admit having longer strings probably does tend to make lookups more
> expensive. In order to avoid duplication you'd probably end up doing
> two lookups:
>
> english -> unique (int?) key (in a common map file)
> unique (int?) key -> nls message (in an nls specific map file)
>
> I guess you'd probably take the pragmatic approach and do:
>
> throw ExceptionManager.create(new IOException(), "TOKEN: Stream is closed");
>
> and strip the token to either look it up or just print the remainder
> of the message.
>
>> > All this is dependent upon the premise that messages are not used
>> > often, I need to check that first.
>>
>> Indeed - this might be overkill if it only has a small impact.
>
> Yeah. We really need to confirm/refute this theory.
>
> Regardless though, I rather like the "no lookups for en/C locale idea"
> because it improves code readability too.
>
> -Mark.
>
> [0] This particular example is a variation of a "bad smell in code" that
> I usual spot that more typically looks like:
>
> if (A || B)
> exit_with_error("Failed because of A or B")
>
> where the error is any string that is independent of A or B.
>
>
>
