Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I see what you mean, however you will have to break the fluidity of the builder to determine if you should set this if it was an int. Whereas if it's an Integer you can use a variable set to null or badge count before calling the builder.

Integer count = determineCount();

AndroidNotification.builder().setNotificationCount(count).build();

Vs

Integer count = ...

Builder builder = AndroidNotification.builder();

if(count != null) {
   builder.setNotificationCount(count);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, is it not the case that if a property annotated with @@key is null, it is not included in the resultant json? Thus fulfilling the requirement of unspecified?

By using a primitive type in the setter, you have no way of indicating this other than not calling the setter.

And so, as a user of the builder, you would then have no way to use the builder fluently if you had to check if the value of notification count was null.

Mindful of the fact that deriving that value might be complex and that the point of calling the builder may have been supplied this value from a higher level of code. Instead of just handing the value to the builder you would have to create a whole branch in code just switching on that value.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, is it not the case that if a property annotated with @@key is null, it is not included in the resultant json? Thus fulfilling the requirement of unspecified?

Right. Set the default value to be null, and you get the right semantics. See how Aps.badge is implemented for an example.

Mindful of the fact that deriving that value might be complex and that the point of calling the builder may have been supplied this value from a higher level of code

The higher-level code should never return null either. It should return 0. If it must return null, it's fairly easy to handle it as:

Integer count = determineCount();
builder.setNotificationCount(count != null ? count : 0); 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be that the higher level code determines that the badge count on the client device is left unchanged by this push notification. The only way to Express this is to return a count == null.

If the fire base builder can handle null properly and convey this by setting notification count to null (such that it is not included in the json). Then what is the problem here?

Null is a valid value for the attribute that says 'leave the count as it is on the client device'

It seems to me that Optional Integer is a good candidate for differentiation, but primitive int is not sufficient to express all values possible without breaking the fluency of the builder.

I can set it to primitive int, no problem, it just does not feel right in this case

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null is a valid value for the attribute that says 'leave the count as it is on the client device'

Null is not a valid value for a field declared as number in the backend. Also in this case the backend allows both 0 and undefined to mean "keep as is". So it's easy to deal with any null-returning user code by mapping null to 0.

Furthermore JSON null and JSON undefined usually have different semantics:

{
  "foo": "test",
  "bar": null // bar is null
}

{
  "foo": "test",
  // bar is undefined
}

When one calls an API as setBar(null), it's typical to expect the value to get serialized as null.

It seems to me that Optional Integer is a good candidate for differentiation

Optional is a Java8 API and we are still on Java7. Plus, that's a somewhat overkill solution for such a simple problem. We already have an established pattern in the SDK to deal with type of fields, and we should be using it here to be consistent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hiranya911

Ok, I think this comes down to a misunderstanding on my part.

My assumption (which is obviously wrong) is that if an attribute annotated with @key is not initialized or set to a specific value. Then its default value in java is null. The assumption here is that your json serializer does not actually add the attribute to the resultant json,

...

@Key("my_var")
private String myVar;

...

i.e. If the value is null, then instead of this:

{ 
  ...
   "my_var": null,
  ...
}

You'd get this:
{
...
...
}