Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why don't you use generic exception inside CheckedFunction? #237

Open
gavlyukovskiy opened this issue Jun 27, 2016 · 5 comments
Open

Why don't you use generic exception inside CheckedFunction? #237

gavlyukovskiy opened this issue Jun 27, 2016 · 5 comments

Comments

@gavlyukovskiy
Copy link

gavlyukovskiy commented Jun 27, 2016

Any use of CheckedFunction requires to use instanceof, but in most cases compiler would do it for us.
Look at the little example:

class Test {
    private static ResultSet throwsSQLException(Statement statement) throws SQLException {
        return null;
    }

    private static String throwsIOException(File file) throws IOException {
        return null;
    }

    private static ResultSet throwsMoreThanOneChecked(File file) throws SQLException, IOException {
        return null;
    }
}

If we want convert throwing SQLException function to unchecked we should write a lot meaningless code using Unchecked:

Unchecked.function(Test::throwsSQLException, t -> {
    if (t instanceof SQLException) {
        throw new UncheckedSQLException((SQLException) t);
    }
    // copy-paste for handling Error, RuntimeException and InterruptedException...
    throw new RuntimeException(t);
});

but using function with generic exception:

public interface CheckedFunction<T, R, THR extends Throwable> {

    R apply(T t) throws THR;

    static <T, R, THR extends Throwable, UCE extends RuntimeException> Function<T, R> checked(
            CheckedFunction<T, R, THR> checked, Function<THR, UCE> exceptionHandler) {
        return (t) -> {
            try {
                return checked.apply(t);
            }
            catch (RuntimeException | Error e) {
                throw e;
            }
            catch (Throwable th) {
                if (th instanceof InterruptedException) {
                    Thread.currentThread().interrupt();
                }
                throw exceptionHandler.apply((THR) th);
            }
        };
    }
}

we can optimize two of three cases.
If methods throws single checked exception compiler will infer type of this exception so we can just write

CheckedFunction.checked(Test::throwsSQLException, UncheckedSQLException::new);
CheckedFunction.checked(Test::throwsIOException, UncheckedIOException::new);

When function throws more than one checked exception compilers can't put two exceptions and requires Throwable to be handled:

CheckedFunction.checked(Test::throwsMoreThanOneChecked, RuntimeException::new);

Also using transforming function protects us from checking than exception was thown and user actually understands what we exactly want.

@lukaseder
Copy link
Member

lukaseder commented Jun 28, 2016

Thank you very much for your suggestion. There's an existing CheckedFunction.unchecked() method that does something similar to what you suggest. However, it doesn't correlate the throw exception from CheckedFunction with the handler. There was a reason for the current design (some limitation related to Java generic exceptions) but I don't remember what it was.

Of course, as a workaround, you could also write a small utility:

Unchecked.function(Test::throwsSQLException, wrap(SQLException.class, UncheckedSQLException::new));

with

static <X extends Throwable> Consumer<Throwable> wrap(Class<X> x, Function<? super X, ? extends RuntimeException> f) {
    return t -> {
        if (x.isInstance(t))
            return f.apply((X) t);
        else
            return new RuntimeException(t);
    };
}

@gavlyukovskiy
Copy link
Author

gavlyukovskiy commented Jun 28, 2016

Thanks for the answer, I think it would be better if I don't lose existing handling of InterruptException:

static <X extends Throwable> Consumer<Throwable> wrap(Class<X> x, Function<? super X, ? extends RuntimeException> f) {
        return t -> {
            if (x.isInstance(t))
                throw f.apply((X) t);
            else
                Unchecked.THROWABLE_TO_RUNTIME_EXCEPTION.accept(t);
        };
    }

Thanks a lot.

@lukaseder
Copy link
Member

Indeed, your suggestion is better. This will also include any future improvement that we add to THROWABLE_TO_RUNTIME_EXCEPTION

@gavlyukovskiy
Copy link
Author

gavlyukovskiy commented Jun 28, 2016

Also keep in mind that using non-generic exception does not allow accepting function permitted for specific exception (please omit the resource closing).

private static ResultSet executeOnAllDatasources(CheckedFunction<Connection, ResultSet> function) throws SQLException {
    SQLException last = null;
    for (DataSource dataSource : dataSources) {
        try {
            return function.apply(dataSource.getConnection());
        }
        catch (SQLException e) {
            last = e;
        }
        catch (Throwable t) {
            // correct handling of unchecked exceptions should be here, but who cares...
            throw new RuntimeException(t);
        }
    }
    throw last;
}
try {
    executeOnAllDatasources(connection -> {
        return connection.prepareStatement("SELECT * FROM table").executeQuery();
    });
}
catch (SQLException e) {
    throw new ServiceException("operation failed on all data sources");
}

in case of adding new checked exception inside function user won't see the effects of it, but if you are using generic exception compiler will show error:

private static ResultSet executeOnAllDatasources(CheckedFunction<Connection, ResultSet, SQLException> function) throws SQLException {
    SQLException last = null;
    for (DataSource dataSource : dataSources) {
        try {
            return function.apply(dataSource.getConnection());
        }
        catch (SQLException e) {
            last = e;
        }
        // no re-throwing Error or RuntimeException, missing InterruptException isn't possible due to restriction
    }
    throw last;
}

But as you said if there are strong limitations (I know only about catching generic exceptions) or some other issues with generic exception it's bad idea.

@lukaseder
Copy link
Member

OK, I can see your point. Indeed, such a change does seem to suggest a much easier handling of wrapping such blocks.

Now, there are only two problems left:

  1. Find out why this wasn't done in the first place (that limitation I've mentioned)
  2. Find out if this really adds enough value to break backwards-compatibility :)

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

No branches or pull requests

2 participants