4
\$\begingroup\$

I am working on an Android app that has an Abstract view controller class. I am using that Abstraction and implementing it for an Android Spinner view. It has a method that assigns the view to be controlled. I implemented it as:

@Override
public void setView(@Nonnull View view) {
    super.setView(view);
    mView = (Spinner) Preconditions.checkNotNull(view);
}

I am getting feedback in a code review that would have me revise the method to:

@Override
public void setView(@Nonnull View view) {
    super.setView(view);

    Preconditions.checkNotNull(view, "The view provided is null");
    Preconditions.checkArgument(view instanceof Spinner,
            "view provided was not a Spinner view");
    mView = Spinner.class.cast(view);
}

I am unclear as to why the second is a better implementation. From what I understand, using Class.cast() is only really useful for generics (1, 2).

Other than that, it looks like the second implementation does the same thing. It will crash the app if view is null, or crash the app if view is not an instance of Spinner.

Is there a benefit to using the Precondition instanceOf check to crash the app in testing rather than a standard ClassCastException?

\$\endgroup\$
2
\$\begingroup\$

This is a relatively new area in Java, and best practice is not as well established....

... but, as I think about it, I think your reviewer has a point.

In your code base, consistency counts for a lot. If you check all your preconditions in a consistent way, and are disciplined about it, it will make a number of things easier to manage and maintain:

  1. logs will be consistent
  2. Exceptions will have more meaningful messages
  3. Programmer's expectations are clearly shown

So, if the suggestions from your reviewer are consistent with the common-practice at your location, then, absolutely, do them.

Even if they are not common practice, still do them.

But, I have some comments to come after a small diversion.....

You are correct that Spinner.class.cast(...) was introduced with Generics. The main purpose of this method appears to be related to the return value of methods with a Generic return type or where generic erasure makes internal manipulation of the data impossible without some help from the class (i.e. it counteracts the effects of erasure).

These methods (or the class they are in) are supplied with an instance of the generic class, and they then use that class instance to cast the value.... for example, in Collections.checkedList(....). It is unusual to see this method (cast()) referenced from a static context Spinner.class.cast(value) when a simple (Spinner)value will suffice.

On the other hand, it is interesting that, if you have multiple values being cast on a single line of code, the ClassCastException stack trace for (Spinner)value will be different to the trace for Spinner.class.cast(value). I need to verify this, but it may make traces slightly more useful.

So, Spinner.class.cast(value) is not exactly the same as (Spinner)value.

Now, having said that, I can understand why Spinner.class.cast(...) may be recommended, but then I would have to question why Preconditions.checkArgument(view instanceof Spinner, ...) was recommended....

The view instanceof Spinner is logical to me, but, given the context of using Spinner.class.cast() I would also suggest that you should use Spinner.class.isInstance(view) as well.

If you are going to use these methods then I would suggest consistency.

Finally, I think that, unless there is the intention to turn this class in to a more generic-based one, that using the cast(...) and isInstance(...) is redundant. I would prefer the traditional (Spinner)view and view instanceof Spinner.

But, I fully agree that if you are going to check pre-conditions, that separating out the precondition check for instanceof is a good idea (although I would add the word 'The' to the beginning of the message).

So, if it were me, I would (given the context), suggest a compromise:

@Override
public void setView(@Nonnull View view) {
    super.setView(view);

    Preconditions.checkNotNull(view, "The view provided is null");
    Preconditions.checkArgument(view instanceof Spinner,
            "The view provided was not a Spinner view");
    mView = (Spinner)view;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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