Wrong parameter annotation or NPE in builder method for optional field

[AndreasBoehm]

foo is annotated @ Nullable in the builder method but in the next line a null-check is executed that will throw a NPE when foo is actually null.

Should we change @ Nullable to @ NonNull or should we adjust the null-check to just not call mArguments.putString in case foo is null?

  public FragmentKotlinBuilder foo(@Nullable String foo) {

    if (foo == null) {
      throw new NullPointerException("Argument 'foo' must not be null.");
    }
    mArguments.putString("foo", foo);
    return this;
  }

[sockeqwe]

Yeah, is related to #83

The idea was to use crash early and allow only non-null values as parameter for optional Args. Tha'ts the reason why this check was involved.

However, in practice this is a little bit cumbersome. Example:

class MyFragment {
   @Arg(required = false) Foo foo;
}

Foo foo = new Foo();
new MyFragmentBuilder()
    .foo(foo) // foo is not null
    .build();

However, since foo is optional, this could also be null.

Foo foo = null;
new MyFragmentBuilder()
    .foo(foo) // foo == null, throws NullPointerException
    .build();

As already said the idea was that in that case a NullPointerException is thrown immediately so that we can track down where the null value comes from (by looking at the stacktrace).

But then you have to write code like this:

MyFragmentBuilder builder = new MyFragmentBuilder();

if (foo != null)
   builder.foo(foo);

builder.build();

While the idea of crashing early to get a clear stacktrace is nice, it is a little bit cumbersome to add null checks when working with the FragmentBuilder.

I tend to revert this change. No Annotation on the parameter and not throwing a NullPointerException like this:

 public FragmentKotlinBuilder foo(String foo) {
    if (foo != null) {
        mArguments.putString("foo", foo);
    } 
    return this;
  }

What do you think?

[AndreasBoehm]

I think its not a good idea to produce runtime crashes because a parameter specified as Nullable was actually null. Also removing the Annotation completely sounds like a step back to me. But this could be me because i rely heavily on Nullable/NonNull Annotations in my projects.

We could keep foo being annotated with @ Nullable but instead of throwing a NullPointerException we should ignore null values.
Regarding #83 this would also mean that we can have the general rule that Nullable fields are optionals. This would help if we want to use @ Nullable and remove the "required" option from the Annotation.

public FragmentBuilder foo(@Nullable String foo) {
    if (foo != null) {
        mArguments.putString("foo", foo);
    } 
    return this;
  }

Otherwise we could say that for optional fields we still only want to allow non-null values.
Additionally to throwing a NullPointerException we should then annotate foo with @ NonNull to make sure the issue of passing something that is possibly null gets spotted during development.

public FragmentKotlinBuilder foo(@NonNull String foo) {
    if (foo == null) {
      throw new NullPointerException("Argument 'foo' must not be null.");
    }
    mArguments.putString("foo", foo);
    return this;
  }

Please let me know what you think :)

[sockeqwe]

I think you are right. I would go with this one:

public FragmentBuilder foo(@Nullable String foo) {
    if (foo != null) {
        mArguments.putString("foo", foo);
    } 
    return this;
  }