2

Below is a Java class constructor which accepts a callback for error handling. null should be an acceptable value, and if the passed value is null then skip error handling.

I have two ways of checking if the callback passed is able to be accepted, and my question here is which method (if either) would be the best for long-term maintainability and sanity. I am open to suggestions as well if anyone believes that there are better ways of going about this, as I am relatively new to using Java.

class ErrorMaker {
    public ErrorMaker(Consumer<Throwable> onError) {
        ClassLoader classloader = Thread.currentThread().getContextClassLoader();
        try (InputStream inputStream = classloader.getResourceAsStream("\\\\..\\..")) {
            Properties config = new Properties();
            config.load(inputStream);
            // ...
        } catch (Exception err) {

            // METHOD #1 - null comparison
            if (onError != null) {
                onError.accept(err);
            }

            // METHOD #2 - instanceof
            if (onError instanceof Consumer<?>) {
                onError.accept(err);
            }

        }
    }
}

Thanks in advance.

7
  • Both are fine and the choice is more a personal one.
    – Turing85
    Commented Apr 24 at 17:35
  • 7
    Personally, I would use method one and would find the instanceof approach confusing (we already know onError is a Consumer when non-null). But that's just my opinion.
    – Slaw
    Commented Apr 24 at 17:45
  • 1
    Although you can use instanceof it's initally not meant to be used for your usecase. To make things clear for other coders in terms of readability and logic you should either go woth classic onError != null or alternatively Objects.nonNull(onError). But if you have to check whether your error is an instance of a specific class then of course instanceof is ok, but you ask for null checking, therefore don't try to trick coders thinking in one way when actual you mean something else.
    – Slevin
    Commented Apr 24 at 18:33
  • 4
    instanceof can be confusing, it is hiding the real purpose - if you want to check for null, just do != null (or similar)
    – user85421
    Commented Apr 24 at 19:04
  • 4
    A different option that might be worth considering: Have two constructors, one with a callback parameter that must be non-null, and one without the parameter, which would then imply no error-handling. Delegate the common work to a final method to avoid code duplication.
    – Anonymous
    Commented Apr 24 at 19:57

1 Answer 1

5

tl;dr

Use Objects class for easier reading.

if ( Objects.nonNull( onError ) ) {…}  // Replaces `if ( onError != null )`. 

Simple & obvious > clever

As the Comments explained, both of your solutions work technically:

  • if (onError != null) {…}
  • if (onError instanceof Consumer<?>) {…}

Of the two, the first is preferable because it is obvious.

The second appears to be testing for one of several possible subtypes. But you are making ever-so-clever use of the fact that instanceof also acts as a test for being non-null. Such clever code is to be avoided, IMHO, because it fails the most critical of tests: Will this code be crystal clear to the exhausted reader’s blurry eyes at 3 AM during an all-night emergency debugging session?

Objects.nonNull( onError )

Personally, I would recommend a third option: the convenience methods on Objects class, isNull, nonNull, and requireNonNull. English words (“nonNull”) are usually easier to read than mathematical signs (“!=“).

if ( Objects.nonNull( onError ) ) {…}  // Replaces `if ( onError != null )`. 

If null were not an acceptable/expected value, call Objects.requireNonNull. This method throws an exception upon encountering a null.

Objects.requireNonNull( x ) ;  // Throws exception if `x` is null. 
1
  • Thank you for your reply and explanations. I believe Objects.nonNull will be the best option for me. Cheers!
    – carrj-nm
    Commented Apr 30 at 13:12

Not the answer you're looking for? Browse other questions tagged or ask your own question.