commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rob Spoor <apa...@icemanx.nl>
Subject Re: [lang] org.apache.commons.text.lookup.IllegalArgumentExceptions to [lang].
Date Wed, 04 Sep 2019 19:24:14 GMT
That Builder can throw an unexpected exception if you forget to call 
withFormat, because the format and args remain null.

Regarding the reflection code, the null checks are unnecessary. 
getDeclaredConstructor cannot return null, it throws an exception 
instead if the constructor cannot be found.


On 04/09/2019 16:22, Gilles Sadowski wrote:
> Le mer. 4 sept. 2019 à 14:34, Xeno Amess <xenoamess@gmail.com> a écrit :
>>
>> No I still don't think it a good idea to have such a util class.
>> 1. most Throwable classes, they all have a constructor with a String.
>> 2. IllegalArgumentException is not special in this way.
> 
> For a library such as [Lang], that seems quite right.
> 
>> 3. If we make a public Util class for IllegalArgumentException, then
>> we shall make similar public Util classes for all other Throwables in
>> JDK.
> 
> Or a single factory class that can create all (untested code):
> 
> ---CUT---
> public class ExceptionFactory {
>      private ExceptionFactory() {}
> 
>      public static IllegalArgumentBuilder illegalArgument() {
>          return new IllegalArgumentBuilder();
>     }
> 
>      private abstract static class Builder<T extends Throwable> {
>          private final String format;
>          private final Object formatArgs;
> 
>          protected Builder() {
>              this(null, null);
>          }
>          private Builder(String format, Object[] formatArgs) {
>              this.format = format;
>              this.args = formatArgs;
>          }
>          public Builder withFormat(String format, Object ... args) {
>             return new Builder(format, args);
>          }
>          protected String formatMessage() {
>              return String.format(format, formatArgs);
>          }
>          protected abstract T build();
>      }
> 
>      public static class IllegalArgumentBuilder extends
> Builder<IllegalArgumentException> {
>          @Override
>          public IllegalArgumentException build() {
>              return new IllegalArgumentException(formatMessage());
>          }
>      }
> }
> ---CUT---
> 
> Usage would be:
> ---CUT---
> throw ExceptionFactory.illegalArgument().withFormat(format, args).build();
> ---CUT---
> 
>> However if you really want to add something like that, I think
>> something like this would be a quite reasonable answer:
>>
>>
>> import org.junit.jupiter.api.Test;
>>
>> import java.lang.reflect.Constructor;
>> import java.lang.reflect.InvocationTargetException;
>> import java.util.Arrays;
>>
>> import static org.junit.jupiter.api.Assertions.assertEquals;
>>
>> public class ExceptionUtils {
>>      public static <T> T generateException(final Class<T>
>> exceptionClass, final String format,
>>                                            final Object... arguments) {
>>          T result = null;
>>          try {
>>              Constructor<T> exceptionConstructor =
>> exceptionClass.getDeclaredConstructor(String.class);
>>              if (exceptionConstructor != null) {
>>                  result =
>> exceptionConstructor.newInstance(String.format(format, arguments));
>>              }
>>          } catch (NoSuchMethodException | InstantiationException |
>> IllegalAccessException | InvocationTargetException e) {
>>              throw generateException(IllegalArgumentException.class, e,
>> "Can't create %s with arguments %s.",
>>                      exceptionClass.getCanonicalName(),
>> Arrays.toString(arguments));
>>          }
>>          return result;
>>      }
>>
>>      public static <T> T generateException(final Class<T>
>> exceptionClass, Throwable cause, final String format,
>>                                            final Object... arguments) {
>>          T result = null;
>>          try {
>>              Constructor<T> exceptionConstructor =
>> exceptionClass.getDeclaredConstructor(String.class, Throwable.class);
>>              if (exceptionConstructor != null) {
>>                  result =
>> exceptionConstructor.newInstance(String.format(format, arguments),
>> cause);
>>              }
>>          } catch (NoSuchMethodException | InstantiationException |
>> IllegalAccessException | InvocationTargetException e) {
>>              throw generateException(IllegalArgumentException.class, e,
>> "Can't create %s with arguments %s.",
>>                      exceptionClass.getCanonicalName(),
>> Arrays.toString(arguments));
>>          }
>>          return result;
>>      }
> 
> Seems fragile to risk generating an exception other than the expected
> one (IIUC).
> 
> Regards,
> Gilles
> 
>>
>>      @Test
>>      public void test() {
>>          RuntimeException re =
>> generateException(RuntimeException.class, "%d,,%s", 1, "2020");
>>          assertEquals(re.getMessage(), "1,,2020");
>>          RuntimeException re2 =
>> generateException(RuntimeException.class, re, "%d,,%s", 2, "3030");
>>          assertEquals(re2.getMessage(), "2,,3030");
>>          assertEquals(re2.getCause(), re);
>>      }
>> }
>>
>> Gilles Sadowski <gilleseran@gmail.com> 于2019年9月4日周三 下午7:17写道:
>>>
>>> Hi.
>>>
>>> Le mer. 4 sept. 2019 à 02:53, Gary Gregory <garydgregory@gmail.com> a
écrit :
>>>>
>>>> Here is the PR with [lang]'s own call sites updated to use the new code.
>>>>
>>>> Gary
>>>>
>>>> On Tue, Sep 3, 2019 at 2:13 PM Gary Gregory <garydgregory@gmail.com>
wrote:
>>>>
>>>>> On Tue, Sep 3, 2019 at 1:19 PM Rob Spoor <apache@icemanx.nl> wrote:
>>>>>
>>>>>> Why limit this to IllegalArgument? Why not make it more generic?
For
>>>>>> instance, in ExceptionUtils:
>>>>>>
>>>>>> public static <T extends Throwable> T format(final Function<?
super
>>>>>> String, ? extends T> factory, final String format, final Object...
args) {
>>>>>>       return factory.apply(String.format(format, args));
>>>>>> }
>>>>>>
>>>>>> public static <T extends Throwable> T format(final BiFunction<?
super
>>>>>> String, ? super Throwable, ? extends T> factory, final Throwable
t,
>>>>>> final String format, final Object... args) {
>>>>>>       return factory.apply(String.format(format, args), t);
>>>>>> }
>>>>>>
>>>>>> These can then be called using
>>>>>> ExceptionUtils.format(IllegalArgumentException::new, "message: %s",
>>>>>> message) or ExceptionUtils.format(IllegalArgumentException::new,
cause,
>>>>>> "message: %s", message).
>>>
>>> API looks a little bit strange (throw a "format"?)
>>>
>>> Perhaps:
>>> ---CUT---
>>> throw IllegalArgumentExceptionFactory.withFormat(...);
>>> ---CUT---
>>>
>>> Is there additional customization foreseen (that may require
>>> a "builder")?
>>>
>>> Also, wouldn't it be useful to not perform the formatting
>>> unconditionally but rather when "getMessage() is called?
>>>
>>> Gilles
>>>
>>>>>>
>>>>>> It's a bit verbose though, but it gives a lot more flexibility.
>>>>>>
>>>>>
>>>>> Yes, we could add these to ExceptionUtils separately IMO; but, the
>>>>> verbosity is an issue for me. This is straightforward:
>>>>>
>>>>> throw IllegalArgumentExceptions.format(e, "%s: %s %s operation %s: %s",
>>>>> source, method, pathSpec, operation.getOperationId(), e.toString());
>>>>>
>>>>> The following not as much:
>>>>>
>>>>> throw ExceptionUtils.format(IllegalArgumentException::new, e, "%s: %s
%s
>>>>> operation %s: %s", source, method, pathSpec, operation.getOperationId(),
>>>>> e.toString());
>>>>>
>>>>> So it could be that IllegalArgumentExceptions.format() is implemented
in
>>>>> terms of ExceptionUtils.format but there does not seem to be much to
gain
>>>>> from that.
>>>>>
>>>>> Gary
>>>>>
>>>>>
>>>>>>
>>>>>> On 03/09/2019 19:04, Gary Gregory wrote:
>>>>>>> Please read the source.
>>>>>>>
>>>>>>> On Tue, Sep 3, 2019, 12:27 Xeno Amess <xenoamess@gmail.com>
wrote:
>>>>>>>
>>>>>>>> Why don't you use java.lang.IllegalArgumentException instead?
>>>>>>>> And, why we need such a class, but not using
>>>>>>>> java.lang.IllegalArgumentException there?
>>>>>>>>
>>>>>>>> Gary Gregory <garydgregory@gmail.com> 于2019年9月3日周二
下午11:18写道:
>>>>>>>>>
>>>>>>>>> Hi All:
>>>>>>>>>
>>>>>>>>> I propose we take
>>>>>>>>>
>>>>>>>>
>>>>>> https://commons.apache.org/proper/commons-text/xref/org/apache/commons/text/lookup/IllegalArgumentExceptions.html#IllegalArgumentExceptions
>>>>>>>>> and make it a public class in [lang]. FWIW, I use a class
like this in
>>>>>>>> many
>>>>>>>>> projects at work.
>>>>>>>>>
>>>>>>>>> Gary

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


Mime
View raw message