Hello Luc.
> >
> >>>> [...]
> >>>> I encountered this need in two different cases. The first one was to
> >>>> identify very precisely an error type, even with finer granularity than
> >>>> exception type. Parsing the error message to recognize the exception is
> >>>> evil, checking the enumerate used in the pattern is less evil. The
> >>>> second case was when I needed to create a more elaborate message by
> >>>> combining some information provided by the caller, and some information
> >>>> extracted from the exception. Here again, parsing is evil but getting
> >>>> the parameters is fine.
> >>>
> >>> Maybe you missed my point (same as above), as it applies here too: You can
> >>> get the parameters through the accessors (of the specific exception
> >>> types).
> >>> We created the "context" so that additional parameters can be set and
> >>> retrieved ("key/value" pairs). I still do not understand why one should
> >>> resort to extract something from the pattern.
> >>> [The pattern is unfortunately "public" whereas it should be an
> >>> "implementation detail".]
> >>
> >> I don't "extract" something from the pattern, I just check if it is a
> >> known and expected enumerate I want to handle specifically or something
> >> else.
> >
> > Maybe I should see the actual code before we go on discussing this. Of
> > course you are free to do whatever the API of CM lets you do. I just have
> > the feeling that I would do it otherwise... :-}
>
> Here is an example I encountered two minutes ago, while working on
> adding the exception throws statements in the ode package.
>
> The computeParameterJacobian may throw a MathIllegalArgumentException
> (in the current subversion repository, I have seen it throws an
> IllegalArgumentException, which is wrong). In the following piece of
> code, I need to check the exception I get is really a
> LocalizedFormats.UNKNOWN_PARAMETER or something else:
>
>
> delayedExcpetion = null;
> for (ParameterJacobianProvider provider: jacobianProviders) {
>
> try {
>
> provider.computeParameterJacobian(...);
> return;
>
> } catch (MathIllegalArgumentException miae) {
> List<Localizable> patterns = miae.getContext().getPatterns();
> if (patterns.contains(LocalizedFormats.UNKNOWN_PARAMETER)) {
> // temporarily ignore, until we have checked all providers
> delayedException = miae;
> } else {
> // this is another problem, report it
> throw miae;
> }
> }
>
> }
>
> if (delayedException != null) {
> // none of the providers did handle the parameters
> throw miae;
> }
>
> Here, I only use only the pattern in the check, sometimes I need to also
> check the parameters (for example here I could use the name of the
> parameter). Note that the exception thrown here is a high level
> MathIllegalArgumentException, not a specialized exception like
> NumberIsTooLarge which does have specialized getters like getMax.
>
> So for this kind of use, I need getters in the context class
> (getPatterns and getParameters). The alternative would be to create
> specialized exceptions for every single LocalizedFormats we have, which
> is clearly too much.
What can I say? :-) IMO, for every such use, there should indeed be a
specific exception class. It's really a pity that a user (you) should do
this gymnastics because CM developers (Oh, that'd be you too! ;-) didn't
want to have a specific type for conveying a specific problem.
"MathIllegalArgumentException" is a high-level abstraction for the usage of
_users_ with the sole purpose that they do not have to duplicate code when
they just want to catch a whole slew of exceptions in one go (by the way,
this is exactly the same rationale as for having a "MathRuntimeException");
developers should not throw this exception but rather a specific one that
describes the exact problem at hand.
If the exception is very "local" to the code, like in the above, you could
have it defined in the package (or as an inner class). With this, your user
code could become
---CUT---
delayedException = null;
for (ParameterJacobianProvider provider: jacobianProviders) {
try {
provider.computeParameterJacobian(...);
return;
} catch (org.apache.commons.math3.ode.UnknownParameterException miae) {
// temporarily ignore, until we have checked all providers
delayedException = miae;
}
}
if (delayedException != null) {
// none of the providers did handle the parameters
throw miae;
}
---CUT---
Simpler (for you, as a user), cleaner (no "if-else", no need to dig into the
internals of CM), more robust (what if I have it my way and we dump
"LocalizedFormats"? ;-D), more efficient (no catching of the wrong
exception, no extracting of the patterns, no list search), more concise
(hence easier to understand).
[Personally, I find it extremely ugly and brittle to rely on a "String" to
convey that a particular condition has occurred. I'm pretty sure that _you_
can do that because you are the one who wrote the CM code (for that reason,
you know what it means). No other CM user could have written a code like you
did above (at least not without having read the source code).]
Thanks for the discussion; I hope I showed you that there is indeed another
way to do it.
Please note that I don't say that there must be one exception for each
"enum" element. But it's not because there are too any of them.
There should be an exception for each "broad" class of problems ("out of
range", "no data", "not positive", etc. plus more "local" ones, once their
usefulness as _type_ is identified, like in your example).
The _displayed_ message can be enhanced by adding contextual information
when instantiating the exception (like the "standard deviation" is "not
positive") but the information meant for display should not be used as a
substitute for a proper type.
There are indeed too many "enum" elements; many of them could just be
deleted. [Some time ago, I had made some steps toward a big cleanup.]
Best,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
