Re: [Orekit Developers] Data-related exceptions

[Guilhem Bonnefille <guilhem.bonnefille@c-s.fr>]

Le 04/07/2018 à 21:41, MAISONOBE Luc a écrit :
>
> "LUGAN, Anne-Laure [FR]" <anne-laure.lugan@airbus.com> a écrit :
>
>> Hello every one,
>>
>> I have been using unchecked exception more and more in the late
>> years... and I have no regret about it!
>>
>> Against my position, I would like to quote that Eclipse can help to
>> remove partially OrekitException. You can tune Eclipse to raise a
>> compilation warning if the exception is not thrown in the method.
>> Still, it means, if we migrate only a few OrekitException to
>> unchecked, that we have to remove the "throws OrekitException" one by
>> one, ie each time Eclipse raises a warning through all hierarchy call.
>>
>> I agree that it would be more helpful to have a few more meaningful
>> Exception than the single OrekitException.
>
> A handful of exceptions is probably useful, but we should not go to
> the extrem of having one exception for each error. This is what was
> selected for Apache Commons Math (after months of harsh discussions
> that I would really not like to have again). It was almost the first
> thing we removed when creating Hipparchus.
>
> My point at that time is that a library should have a top level exception
> (or two, one for checked and one for unchecked exceptions) so people
> developing top level applications and not aware of a rich exceptions
> hierarchy can still do a "catch (TheTopLevelException e)" if they want,
> and they will catch everything. As some of our algorithms depend on a
> large number of lower level algorithms, using too many dedicated
> exceptions
> would lead people using for example orbit determination to need to catch
> errors linked to propagation, time, force models, JPL ephemerides, EOP,
> measurements, frames, data loading... Using the standard Java hierarchy
> (IOException, ArrayIndexOutOfBoundException, ParseException, ...) would
> prevent such a catch all strategy (there's no multiple inheritance in
> Java), so I am not comfortable with this choice.

As an old developer, I can remember some QA guidelines requiring to
create a root exception dedicated to each project.
But even with many many years of coding, I still do not understand the
motivation of this. Humh... saying it differently: I feel the motivation
(quite similar to the one Luc expressed) is a sort of anti-pattern. As a
developer, at a given level in my code, if I don't care of exceptions, I
don't care of ANY exception: I will not catch the root OrekitException
and then catch separately any other possible exceptions. If I want to do
that (for example because I'm at the top level of the Thread run method)
I will do a cath(Exception) and justify such choice.
For what I saw, such recommandation create duplication of code because
standard exceptions exist for many situations. Furthermore, letting
developper reinvent a tree of exceptions, without trying to reuse and
understand the standard exception tree, is counter-productive.

But this feeling is strictly mine, based on my own experience. And
people on this list have certainly an other experience.

Creating a good (helpfull) exceptions is hard, really hard, requiring
both a good knowledge of the internal code, but also a good knowledge of
how it will be used. Is it really exceptional case? Do I need an
exception or a specific return value? Does the caller respect the
contract of the class? ...

> As a summary, I think we have seen the following proposals in this
> thread:
>
>
>  1) change a few checked exceptions to unchecked
>  2) change all checked exceptions to unchecked
>  3) use standard java exceptions (IOException, ...)
>  4) create a few different Orekit exception for different errors
>  5) use a small Orekit hierarchy with an easy to catch top level
>
> Recalling previous messages I would say the various positions are:
>
>  Yannick   :  1 + ?
>  Luc       :  hesitating between 1 + 4 + 5 or 2 + 4 + 5
>  Evan      :  2 + ?
>  Guilhem   :  ? + 3
>  Anne-Laure:  2 + 4
>

Based on my few experience of Orekit, I remember there are some
situations where checked exception can be turned into unchecked one.
Generaly, this is for the code that can be qualified as "helper" code,
masking lot of complexity. In such situation, the caller is not directly
responsible of the exceptional situations and he is unable to fix
anything -> Runtime.

Furthermore, is it only a question of checked/unchecked exceptions? Why
do we need lot of different exceptions? What about using log traces to
give developer/user some feedback about what brings to an empty result?

>> Best regards,
>>
>> Anne-Laure
>>
>> -----Original Message-----
>> From: orekit-developers-request@orekit.org
>> [mailto:orekit-developers-request@orekit.org] On Behalf Of Guilhem
>> Bonnefille
>> Sent: Monday, July 02, 2018 5:44 PM
>> To: orekit-developers@orekit.org
>> Subject: Re: [Orekit Developers] Data-related exceptions
>>
>>
>>
>> Le 02/07/2018 à 14:35, MAISONOBE Luc a écrit :
>>>
>>> "Ward, Evan" <Evan.Ward@nrl.navy.mil> a écrit :
>>>
>>>> Hi Yannick,
>>>>
>>>> On Mon, 2018-07-02 at 08:56 +0000, JEANDROZ, Yannick [FR] wrote:
>>>>
>>>> I believe this change would allow for a more lightweight code for
>>>> Orekit users.
>>>> What are your thoughts about this proposition ? If there is a
>>>> consensus, I could push the analysis further and begin to tinker with
>>>> Orekit code.
>>>>
>>>> I am OK with making OrekitException a RuntimeException.
>>>
>>> Wow, this would be a drastic change! For sure, it would solve Yannick
>>> (and many others) problem.
>>>
>>> What I like in checked exception is that you don't get surprised, the
>>> compiler prevents you from creating code that does not check for
>>> errors.
>>> However, I do agree that this can go out of hands if the code has many
>>> internal checks and errors can be raised almost everywhere. So I admit
>>> that we failed at some points and that as Yannick writes, almost every
>>> method throws an OrekitException. Therefore, the current code is
>>> crippled with throws declaration in too many places.
>>>
>>> So... I'm on the fence on being convinced to follow Evan drastic
>>> suggestion.
>>> What do other people think about this? Could someone push a little
>>> harder to convince old school developers like me?
>>>
>>> If we go this way, should we directly remove the "throws
>>> OrekitException"
>>> declarations and remove the corresponding Javadoc? I think we should,
>>> because with unchecked exceptions, the tools (IDE, compiler,
>>> checkstyle...)
>>> will not help us maintain the consistency of such declarations, and it
>>> will soon become inconsistent with underlying code.
>>>
>>
>> I share the Luc's feeling: checked Exceptions are not bad by design and
>> cannot be replaced by unchecked ones.
>>
>> In order to gain full benefit of exceptions, they require lot of
>> attention, a precise comprehension of use cases and a neat design.
>>
>> I feel that the current issue is that Orekit should use a more refined
>> tree of exceptions or use native/standard exceptions, in order to
>> distinguish different situations and allow each conceptual level to
>> decide what to do with witch situation. But this needs lot of work (575
>> occurrences of "throw new OrekitException").
>>
>> For example, many classes related to file parsing should throw native
>> IOException (or inherited ones). When dealing directly with the parser,
>> a developer needs to handle the problem:
>> - the file does not exist in the current path, so perhaps can I look for
>> it elsewhere.
>> - the syntax is wrong? I can ignore the file and look for an other one.
>> But higher level classes, involving the previous one, should certainly
>> decide to wrap lower level exceptions into unchecked ones.
>> As a developer, when dealing with TimeScales, I don't care about
>> IOException and, as spotted by Yannick, I'm strictly unable to do
>> anything at this level.
>>
>> Another way to decide if an exception must be checked or unchecked is to
>> consider the *contract* of the method. If the caller does not respect
>> the contract, then we can throw an unchecked one, examples: the
>> NullPointerException, or the design of the Iterator where the caller is
>> expected to use hasNext() before calling next().
>>
>> Hope That Helps.
>>
>> ***************************************************************
>> Ce courriel (incluant ses eventuelles pieces jointes) peut contenir
>> des informations confidentielles et/ou protegees ou dont la diffusion
>> est restreinte. Si vous avez recu ce courriel par erreur, vous ne
>> devez ni le copier, ni l'utiliser, ni en divulguer le contenu a
>> quiconque. Merci d'en avertir immediatement l'expediteur et d'effacer
>> ce courriel de votre systeme. Airbus Defence and Space et les
>> sociétés Airbus Group declinent toute responsabilite en cas de
>> corruption par virus, d'alteration ou de falsification de ce courriel
>> lors de sa transmission par voie electronique.
>> This email (including any attachments) may contain confidential
>> and/or privileged information or information otherwise protected from
>> disclosure. If you are not the intended recipient, please notify the
>> sender immediately, do not copy this message or any attachments and
>> do not use it for any purpose or disclose its content to any person,
>> but delete this message and any attachments from your system. Airbus
>> Defence and Space and Airbus Group companies disclaim any and all
>> liability if this email transmission was virus corrupted, altered or
>> falsified.
>> ---------------------------------------------------------------------
>> Airbus Defence and Space SAS (393 341 516 RCS Toulouse) - Capital:
>> 29.821.072 EUR - Siege social: 31 rue des Cosmonautes, ZI du Palays,
>> 31402 Toulouse cedex 4, France

begin:vcard
fn:Guilhem Bonnefille
n:Bonnefille;Guilhem
org;quoted-printable:CS Syst=C3=A8mes d'Information;Division Espace
adr:;;Parc de la Grande Plaine - 5, rue Brindejonc des Moulinais - BP 15872;Toulouse Cedex 05;;31506;France
email;internet:guilhem.bonnefille@c-s.fr
title:Architecte
tel;work:05 61 17 62 84
url:http://www.c-s.fr/
version:2.1
end:vcard