Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Support for Guava Optional #79

Merged
merged 3 commits into from
Oct 5, 2014
Merged

Support for Guava Optional #79

merged 3 commits into from
Oct 5, 2014

Conversation

drekbour
Copy link
Contributor

@drekbour drekbour commented Oct 1, 2014

Resolution for #66

Please don't accept or reject this PR as it is not done (unless you are going to append the fix for the below after accepting!).

It is 99% complete (with test) except that, inside the processor the withOptionals directive is exposed as some horrible compiler symbol not the original enum. I remap by toString back into the OptionalSupport enum but it feels very wrong like that.

Please advise on Directives.java:35

* @throws Exception
*/
@Test
public void testShouldNotGenerateGuavaOptionalsForOptionalMembers() throws Exception {
Copy link
Owner

Choose a reason for hiding this comment

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

Good point :-)

@mkarneim
Copy link
Owner

mkarneim commented Oct 1, 2014

Thank you for this!
I'll merge this soon.
Just out of curiosity: can you give me some short realistic use case that uses this?

@drekbour
Copy link
Contributor Author

drekbour commented Oct 1, 2014

  1. The general gist of Optional is to avoid the need for null-defensive coding everywhere without replacing it by more code than you took out.
  2. There may sometimes be a penalty to calling a setter with a null value. For instance if a property already had a default assigned at construction that you overwrite with null because a lookup fails. With the Optional support, the setter is not called for an absent value.

Assume in the following that a request may be authenticated or anonymous (no uid/pwd). It would have taken a dozen lines to do this if nothing was using Optional:

public static final Optional<String> jsonPath( String json, String path );

new RequestBuilder()
  .withUserId(jsonPath(input, "$.owner.uid")) // Invisibly passes the Optional through
  .withPassword(jsonPath(input, "$.owner.pwd").transform(decryptionFunction)) // ignores absent passwords
  .build();

@mkarneim
Copy link
Owner

mkarneim commented Oct 3, 2014

Marc,

I just pushed an alternative variant of this to https://github.com/mkarneim/pojobuilder/tree/drekbour-optional-guava. Essentially I removed the OptionalSupport enum. Instead you can use the Optional class itself.

See this example (from your test case):

package net.karneim.pojobuilder.processor.with.optionals;
import net.karneim.pojobuilder.GeneratePojoBuilder;
import com.google.common.base.Optional;

@GeneratePojoBuilder(withOptionalProperties = Optional.class)
public class PojoWithGuavaOptional {

  public int primitiveInt;
  public Integer boxedInt;
  public int[] array;

}

In order to use Optional from Java 8 just replace the import:

package net.karneim.pojobuilder.processor.with.optionals;
import net.karneim.pojobuilder.GeneratePojoBuilder;
import java.util.Optional;

@GeneratePojoBuilder(withOptionalProperties = Optional.class)
public class PojoWithGuavaOptional {

  public int primitiveInt;
  public Integer boxedInt;
  public int[] array;

}

Any comments?

@drekbour
Copy link
Contributor Author

drekbour commented Oct 3, 2014

I'd considered doing exactly that so am quite happy you found the same solution. It is also nice as it guarantees your chosen Optional is indeed available at compile time. I think we're done here.

@mkarneim mkarneim merged commit c69d1cf into mkarneim:master Oct 5, 2014
@mkarneim mkarneim modified the milestone: 3.2.0 Oct 5, 2014
@drekbour drekbour deleted the optional-guava branch March 14, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants