commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [math] Exception Design
Date Wed, 23 Dec 2015 13:32:04 GMT
Hello.

On Wed, 23 Dec 2015 10:38:03 +0100, luc wrote:
> Le 2015-12-23 01:41, Gilles a écrit :
>> On Tue, 22 Dec 2015 13:17:16 -0600, Ole Ersoy wrote:
>>> On 12/22/2015 11:46 AM, Gilles wrote:
>>>> Hi.
>>>> On Mon, 21 Dec 2015 22:44:16 -0600, Ole Ersoy wrote:
>>>>> On 12/21/2015 06:44 PM, Gilles wrote:
>>>>>> On Mon, 21 Dec 2015 12:14:16 -0600, Ole Ersoy wrote:
>>>>>>> Hi,
>>>>>>> I was considering jumping into the JDKRandomGenerator exception
>>>>>>> discussion, but I did not want to hijack it.
>>>>>>> Not sure if any of you have had a chance to looks at this:
>>>>>>> https://github.com/firefly-math/firefly-math-exceptions/
>>>>>>>
>>>>>>>
>>>>>>> 
>>>>>>> https://github.com/firefly-math/firefly-math-exceptions/blob/master/src/main/java/com/fireflysemantics/math/exception/MathException.java
>>>>>> [...]
>>>>>> But I did not see how localization is handled.
>>>>> I did leave localization out.  I think localization was a hard
>>>>> requirement in earlier versions of CM, but I'm hoping that there 
>>>>> is
>>>>> some flexibility on this
>>>> There was not, since I argued many times to leave it out.
>>>> So unless you can show practically how it can work, I have my 
>>>> doubts
>>>> that we'll be allowed to go forward with this approach.
>>>>
>>>>> and that future versions can defer to a
>>>>> utility that uses the ExceptionTypes Enum instance as the key to 
>>>>> look
>>>>> up the internationalized template string.
>>>> Looks good.  Where is the code? ;-)
>>> So CM clients would:
>>> catch(MathException e) {
>>>     String exceptionTemplate = 
>>> ResourceBundle.getBundle("cm.exception.templates", new Locale("en", 
>>> "US")).getString(e.getType());
>>>     String i18Nmessage = buildMessage(exceptionTemplate, 
>>> e.getContext());
>>>     ...
>>> }
>>> I can prototype that out more.  Just trying to get a feel for how
>>> viable the concept is first.
>> I'm not sure I understand correctly.
>> Does that mean that
>> 1. Uncaught exceptions will lead to a message in English?
>> 2. Every "catch" must repeat the same calls (although the arguments 
>> are likely
>>    to be the same for all clauses (and for all applications)?
>> Comparing this with the current behaviour (where the translated 
>> message String
>> is created when "e.getLocalizedMessage()" is called) is likely to 
>> make people
>> unhappy.
>
> This could be made simpler with some task delegating between user
> code and CM code.
> What about :
>
>  interface ExceptionLocalizer {
>     /** Localize an exception message.
>       * @param locale locale to use
>       * @param me exception to localize
>       * @return localized message for the exception
>       */
>     String localize(Locale locale, MathException me);
>  }
>
> and having ExceptionFactory hold a user-provided implementation of
> this interface?
>
>  public class ExceptionFactory {
>
>    private static ExceptionLocalizer localizer = new NoOpLocalizer();
>
>    public static setLocalizer(ExceptionLocalizer l) {
>       localizer = l;
>    }

I think that this is potentially dangerous for two reasons (if I'm
not mistaken): it's not thread-safe and it can be called by any
library used by the main application.

I think that the "localizer" instance must be obtained in a way which
the end-user controls.

>
>    public static String localize(Locale locale, MathException me) {
>      return localizer.localize(locale, me);
>    }
>
>    /** Default implementation of the localizer that does nothing. */
>    private static class NoOpLocalizer implements ExceptionLocalizer {
>           /** {@inheritDoc} */
>           @Override
>           public String localize(MathException me) {
>            return me.getMessage();
>          }
>    }
>
>  }
>
> and MathException could implement both getLocalizedMessage() and
> even getMessage(Locale) by delegating to the user code:
>
>   public class MathException {
>
>     public String getLocalizedMessage() {
>       return ExceptionFactory.localize(Locale.getDefault(), this);
>     }
>
>     public String getMessage(Locale locale) {
>       return ExceptionFactory.localize(locale, this);
>     }
>
>     ...
>
>   }
>
> One thing that would be nice would be that in addition to the get 
> method,
> MathException also provides a getKeys to retrieve all keys and a 
> getType
> to retrieve the type. The later correspond to the getPatern (or
> getLocalizable)
> I asked for a few years ago in ExceptionContext. The point for these 
> methods
> is that if we provide users a way to retrieve the parameters that 
> were used
> in the exception construction, then we can delegate localization to 
> users
> as they can do their own code that will rebuild a complete meaasage 
> as they
> want. When you have only the keys and they have reused names like 
> OPERATOR
> or VECTOR, it can't be delegated.

If those are available (as suggested in Ole's example above), would you 
indeed
be OK that localization is not a CM concern anymore?

We could provide code of how to perform the translation in the 
userguide.

> Note that this is independent of the fact there is one or several
> hierarchies.
> If there are several ones, the two one-liners above must simply be 
> copied
> (yeah, code duplication, I know).
>
>>
>>>>>>
>>>>>>> I think it satisfies everyone's requirements with:
>>>>>>> - A single MathException (No hierarchy)
>>>>>> That would not satisfy everyone. :-!
>>>>>>
>>>>>>> - The ExceptionTypes Enum contains all the exception types
>>>>>>> - The ExceptionTypes Enum 'key' maps to the corresponding 
>>>>>>> message 1 to 1
>>>>>>> - The ExceptionFactory (Utility) throws exceptions, if 
>>>>>>> necessary,
>>>>>>> that have always have a single unique root cause, such as NPEs
>>>>>> I was wondering whether the "factory" idea could indeed satisfy
>>>>>> everyone.
>>>>>> Rather than throwing the non-standard "MathException", the 
>>>>>> factory would
>>>>>> generate one of the standard exceptions, constructed with the 
>>>>>> internal
>>>>>> "MathException" as its cause:
>>>>> I think it's good that CM throws CM specific exceptions.  This 
>>>>> way
>>>>> when I write the handler I can know that the exception is CM 
>>>>> specific
>>>>> without having to unwrap it.
>>>> But if there are several CM exceptions hierarchies, the handler 
>>>> will have
>>>> to check for every base type, leading to more code.
>>> True dat - but if there are no CM exception hierarchies then they 
>>> don't :).
>> I'd be interested in settling the matter of whether we must use a 
>> single
>> hierarchy because of technical limitations, or if it is a good 
>> solution
>> on its own, i.e. extending standard exceptions is not a better 
>> practice
>> after all.
>>
>>>> We could provide a utility:
>>>> public boolean isMathException(RuntimeException e) {
>>>>   if (e instanceof MathException) {
>>>>     return true;
>>>>   }
>>>>   final Throwable t = e.getCause();
>>>>   if (t != null) {
>>>>     if (e instanceof MathException) {
>>>>       return true;
>>>>     }
>>>>   }
>>>>   return false;
>>>> }
>>> Or just not wrap.
>> Of course, but choosing one or the other is not a technical problem;
>> it's design decision.  Do we have arguments (or reference to them)?
>>
>>>>>> public class ExceptionFactory {
>>>>>> public static void throwIllegalArgumentException(MathException 
>>>>>> e) {
>>>>>>     throw new IllegalArgumentException(e);
>>>>>>   }
>>>>>> public static void throwNullPointerException(MathException e) {
>>>>>>     throw new NullPointerException(e);
>>>>>>   }
>>>>>> // And so on...
>>>>>> }
>>>>>> So, CM code would be
>>>>>> public class Algo {
>>>>>> public void evaluate(double value) {
>>>>>>     // Check precondition.
>>>>>>     final double min = computeMin();
>>>>>>     if (value < min) {
>>>>>>       final MathException e = new 
>>>>>> MathException(NUMBER_TOO_SMALL).put(CONSTRAINT, min).put(VALUE, 
>>>>>> value);
>>>>>>       ExceptionFactory.throwIllegalArgumentException(e);
>>>>>>     }
>>>>>> // Precondition OK.
>>>>>>   }
>>>>>> }
>>>>> Another thing that I hinted to is that the the factory builds in 
>>>>> the
>>>>> precondition check in the throw method.  So that the line:
>>>>> if (value < min) {
>>>>> can be nixed.
>>>> It seems nice to ensure that the exception raised is consistent 
>>>> with the
>>>> checked condition.
>>> That's the idea.
>> OK, but do you foresee that all precondition checks will be handle 
>> by
>> factory methods.
>> It would not be so nice to have explicit checks sprinkled here and 
>> there.
>>
>>>> Then, a factory method like
>>>>   throwNotStrictlyPositiveException(Number value, String key)
>>>> should probably be renamed to
>>>>   checkNotStrictlyPositiveException(Number value, String key)
>>> 'check' is good.  I'm going to change it to check.
>
> as the name was changed to checkSomething, the last part Exception in
> the name could be dropped.
>
>>>> Also, shouldn't the "key" argument should be optional?
>>> The key is used to initialize the exception context with the Number
>>> instance.  Different modules could have different keys.  For 
>>> example
>>> the Arithmetic module has keys X and Y.  So if Y caused the 
>>> exception
>>> then Y would be passed as the key.  So if we are checking both we
>>> would do this:
>>> checkNotStrictlyPositiveException(x, X);
>>> checkNotStrictlyPositiveException(y, Y);
>>>>
>>>>>> Then, in an application's code:
>>>>>> public void appMethod() {
>>>>>>   // ...
>>>>>> // Use CM.
>>>>>>   try {
>>>>>>     Algo a = new Algo();
>>>>>>     a.evaluate(2);
>>>>>>   } catch (IllegalArgumentException iae) {
>>>>>>     final Throwable cause = iae.getCause();
>>>>>>     if (cause instanceof MathException) {
>>>>>>       final MathException e = (MathException) cause;
>>>>>> // Rethrow an application-specific exception that will make more

>>>>>> sense
>>>>>>       // to my customers.
>>>>>>       throw new InvalidDataInputException(e.get(CONSTRAINT), 
>>>>>> e.get(VALUE));
>>>>>>     }
>>>>>>   }
>>>>>> }
>>>>>> This is all untested.
>>>>>> Did I miss something?
>
> In the code above, if the iae does not have a cause, or if it is not
> a MathException,
> the iae should be rethrown.

Indeed!
The updated code (also unstested):

    try {
      Algo a = new Algo();
      a.evaluate(2);
    } catch (IllegalArgumentException iae) {
      final MathException e = ExceptionFactory.getMathException(iae);

      if (e != null) {
        // Rethrow an application-specific exception that will make more 
sense
        // to my customers.
        throw new InvalidDataInputException(e.get(CONSTRAINT), 
e.get(VALUE));
      } else {
        throw iae;
      }
    }

>>>>> I think you got it all...But the handler will be shorter if the
>>>>> exception is not wrapped.
>>>> But not significantly, I guess.
>>>> We could also provide
>>>> public MathException getMathException(RuntimeException e) {
>>>>   if (e instanceof MathException) {
>>>>     return (MathException) e;
>>>>   }
>>>>   final Throwable t = e.getCause();
>>>>   if (t != null) {
>>>>     if (e instanceof MathException) {
>>>>       return (MathException) e;
>>>>     }
>>>>   }
>>>>   return null;
>>>> }
>>>> And then define the other utility as:
>>>> public boolean isMathException(RuntimeException e) {
>>>>   return getMathException(e) != null;
>>>> }
>>>>
>>>>> The pattern I'm used to is that libraries
>>>>> wrap the exceptions of other libraries in order to offer a
>>>>> standardized facade to the user.  For example Spring wraps 
>>>>> Hibernate
>>>>> exceptions, since Spring is a layer on top of Hibernate and other 
>>>>> data
>>>>> access providers.
>>>> What do they advertize?  Standard exception, possibly extended, or
>>>> specific ones, possibly belonging to single hierarchy?
>>> Spring namespaced - single hierarchy:
>>> 
>>> http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/dao/DataAccessException.html
>>> BTW - this is blue sky thinking - but Spring Boot has an
>>> @ExceptionHandler annotation that allows the developer to wire up 
>>> an
>>> exception handler.  It might be worth exploring something similar 
>>> for
>>> the purpose of automating I18N requirements.
>>> @ExceptionHandler(MathException.class)
>>> someClientCodeThatUsesCM();
>>
>> That would be quite necessary I'm afraid. ;-)
>
> Not necessarily. The above support for I18N is quite simple.

Maybe too simple... ;-)
[What did they say about global variables?]

Best regards,
Gilles

>
> best regards,
> Luc
>
>> Best regards,
>> Gilles
>>
>>> Cheers,
>>> Ole


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message