Skip to content

Eid exceptions do not extend their Java counterparts, maybe they should? #11

Description

@cardil

Actually Eid exceptions do not extend their Java counterparts.

It's good idea to rethink, is that the right way.

Given for ex instanceof EidIllegalStateException, it's basically to chose what's more beneficial:

ex instanceof EidRuntimeException

or

ex instanceof IllegalStateException

We can't have both. There is also EidContainer interface, thus it can be like:

class EidIllegalStateException extends IllegalStateException implements EidContainer

Activity

  1. changed the title [-]Eid exceptions do not extend their Java counterparts[/-] [+]Eid exceptions do not extend their Java counterparts, maybe they should?[/+] on Oct 31, 2018
  2. adamfabijanczyk commented on Nov 4, 2018

    @adamfabijanczyk

    I think that API of any library should be as most obvious as possible for programmer. When I first saw EidIllegalStateException usage in code, I was pretty sure that it is kind of IllegalStateException. When I jump into implementation of Eid*Exception I was surprised that it is not extending one of common java exceptions.

    So in my opinion EId exceptions should extend their Java counterparts.

  3. cardil commented on Nov 5, 2018

    @cardil
    MemberAuthor

    I thinking the same now.

    Closing on behalf of #13

  4. cardil commented on Nov 28, 2018

    @cardil
    MemberAuthor

    Today I've run on some issue with this changed approach. Consider the following code:

    try {
      return runSomeRiskyOperation();
    } catch (RuntimeException ex) {
      Eid eid;
      if (ex instanceof EidContainer) {
        eid = ((EidContainer) ex).getEid();
      } else {
        eid = new Eid("20181109:174712");
      }
      throw new EidIllegalStateException(eid, ex);
    }

    On line if (ex instanceof EidContainer) { Sonarqube complains with raising squid:S1193

    Using current approach I can write this code as (much more tidy in my opinion):

    try {
      return runSomeRiskyOperation();
    } catch (EidRuntimeException ex) {
      throw ex;
    } catch (RuntimeException ex) {
      throw new EidIllegalStateException(new Eid("20181109:174712"), ex);
    }

    Your thoughts? @adamfabijanczyk

  5. reopened this on Nov 28, 2018
  6. cardil commented on Dec 3, 2018

    @cardil
    MemberAuthor

    Inheriting directly from Java's specific run time exceptions gives us nothing good. Users shouldn't catch specific version of run time exception, ever. Inheriting from specific Java's run time exceptions gives us only one advantage, that is it will be more obvious in that way (for ex.: class EidIllegalStateException extends IllegalStateException).

    Ingeriting from specific Java's run time exceptions came with a cost. That cost was described in my post above. Addtionally, it's handy to catch all Eid exceptions in error handler. That's a real use case.

    Taking all that into account, I'm inclined to leave the current behavior in place. Please, convince me I'm wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions