Skip to content
New issue

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

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

Already on GitHub? # to your account

Add Value#asXXX(defalutValue) for primary types #481

Merged
merged 3 commits into from
Apr 9, 2018

Conversation

zhenlineo
Copy link
Contributor

Add Value#computeOrDefault(Function<Value, T> mapper, T defaultValue) for other types
Add Value#computeOrNull(Function<Value, T> mapper) for shortcuts to call Value#computeOrDefault(Function<Value, T> mapper, T defaultValue=null)

Add Value#computeOrDefault(Function<Value, T> mapper, T defaultValue) for other types
Add Value#computeOrNull(Function<Value, T> mapper) for shortcuts to call Value#computeOrDefault(Function<Value, T> mapper, T defaultValue=null)
@zhenlineo zhenlineo requested a review from lutovich April 3, 2018 14:36
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@zhenlineo Changes look good to me. Made couple minor comments. Methods with default value should also be added for native temporal types from java.time:

  • LocalDate asLocalDate(LocalDate defaultValue)
  • OffsetTime asOffsetTime(OffsetTime defaultValue)
  • LocalTime asLocalTime(LocalTime defaultValue)
  • LocalDateTime asLocalDateTime(LocalDateTime defaultValue)
  • ZonedDateTime asZonedDateTime(ZonedDateTime defaultValue)

/**
* @return the value as a Java boolean, if possible.
* @throws Uncoercible if value types are incompatible.
*/
boolean asBoolean();

/**
* @param defaultValue return this value if the value is a {@link NullValue}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add description and @throws Uncoercible if value types are incompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in the javadoc.

@@ -212,6 +237,12 @@
*/
String asString();

/**
* @param defaultValue return this value if the value is null.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add description and @throws Uncoercible if value types are incompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* @return the value as a Java double.
* @throws LossyCoercion if it is not possible to convert the value without loosing precision.
* @throws Uncoercible if value types are incompatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line should probably be removed

* @return the value as a Java float.
* @throws LossyCoercion if it is not possible to convert the value without loosing precision.
* @throws Uncoercible if value types are incompatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line should probably be removed

@zhenlineo
Copy link
Contributor Author

@sarmbruster Given we have general methods
Values#computeOrDefault(Function<Value, T> mapper, T defaultValue) and Value#computeOrNull(Function<Value, T> mapper)
for all types to use,

On which level do you suggest to have asXXX(defaultVal) methods?

  1. primary types + String only,
  2. all Java Native types
  3. all types that are not Node, Path and Rel
  4. other

@sarmbruster
Copy link
Contributor

sarmbruster commented Apr 5, 2018

Hi @zhenlineo,
what's the difference between primary and native types? (1 and 2)
I basically want defaults to be supported for all property values. So String, primitive types, spatial and temporal types, and arrays

@zhenlineo
Copy link
Contributor Author

zhenlineo commented Apr 5, 2018

@sarmbruster I mean primitive types.

So basically I guess you are asking for the following API:

asString(String def);

// primitives 
asBoolean(boolean def);
asInt(int def);
asLong(long def);
asFloat(float def);
asDouble(double def);

asNumber(Number def);

// array, list, map
asByteArray(byte[] def);
asList(List def);
asMap(Map def);

asObject(Object def);

// node, rel, path
asEntityOrNull();
asNodeOrNull();
asRelationshipOrNull();
asPathOrNull();

// asEntity(Entity entity)   ===> Node + Rel = Entity, Node, Rel, and Path cannot be created so only default to null
// asNode(Node node)
// asRelationship(Relationship rel)
// asPath(Path path)

// spatial, temporal
asPoint(Point def);
asLocalDate(LocalDate d);
asOffsetTime(OffsetTime t);
asLocalTime(LocalTime t);
asLocalDateTime(LocalDateTime d);
asZonedDateTime(ZonedDateTime d);
asIsoDuration(IsoDuration d);

But how about I give you a smaller API:

asString(String def);

// primitives
asBoolean(boolean def);
asInt(int def);
asLong(long def);
asFloat(float def);
asDouble(double def);

computeOrDefault(Function<Value, T> mapper, T defaultValue)
computeOrNull(Function<Value, T> mapper)

// The following are equivalent:
// Point point = pointValue.asPoint(myDefaultPoint) 
// Point point = pointValue.computeOrDefault(Value::asPoint, myDefaultPoint)

// We also have an extra helper method:
// Point pointOrNull = pointValue.computeOrNull(Value::asPoint);

Which API is more useful to you (most of users)? or how would you improve one of them?

@lutovich
Copy link
Contributor

lutovich commented Apr 5, 2018

Imho a slightly trimmed version 1 would be great:

asString(String def);

// primitives
asBoolean(boolean def);
asInt(int def);
asLong(long def);
asFloat(float def);
asDouble(double def);

// array, list, map
asByteArray(byte[] def);
asList(List def);
asMap(Map def);

// spatial, temporal
asPoint(Point def);
asLocalDate(LocalDate def);
asOffsetTime(OffsetTime def);
asLocalTime(LocalTime def);
asLocalDateTime(LocalDateTime def);
asZonedDateTime(ZonedDateTime def);
asIsoDuration(IsoDuration def);

@sarmbruster
Copy link
Contributor

+1 on @lutovich 's previous commit. I guess we should have a asString(String def) as well - it seems to be missing in the list.
These asXXXX plus asString would IMHO cover >90% of all uses.

For the remainder having a type agnostic computeOrDefault is a good idea. computeOrNull is IMHO not needed since it's just as computeOrDefault, mapper, null).

@lutovich lutovich merged commit 7c5570d into neo4j:1.6 Apr 9, 2018
@lutovich lutovich deleted the 1.6-or-default branch April 9, 2018 09:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants