harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.com>
Subject Re: [classlib] Looking up exception messages
Date Thu, 08 Oct 2009 22:16:04 GMT

In message <4ACE0D32.3060209@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.

> >> 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.
   
>  - 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.



Mime
View raw message