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.

Sign up for GitHub

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

[API-8] Improve Tristate #2137

Merged
merged 6 commits into from
Jul 2, 2020
Merged

[API-8] Improve Tristate #2137

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d042657
Improve Tristate:
ItsDoot Jun 30, 2020
3a2ef17
Edit Tristate#not javadocs
ItsDoot Jun 30, 2020
f932920
Simplify Tristate.TRUE#and
ItsDoot Jun 30, 2020
f28569b
* Tristate#fromNullable -> Tristate#fromNullableBoolean
ItsDoot Jun 30, 2020
818b463
Added an extra note to Tristate#asNullableBoolean about the nullable …
ItsDoot Jun 30, 2020
35ba0c0
Grammar
ItsDoot Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions src/main/java/org/spongepowered/api/util/Tristate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,27 @@
*/
package org.spongepowered.api.util;

import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Represents a simple tristate.
*/
public enum Tristate {
TRUE(true) {
@Override
public Tristate and(Tristate other) {
return other == TRUE || other == UNDEFINED ? TRUE : FALSE;
return other == FALSE ? FALSE : TRUE;
}

@Override
public Tristate or(Tristate other) {
return TRUE;
}

@Override
public Tristate not() {
return FALSE;
}
},
FALSE(false) {
@Override
Expand All @@ -49,6 +56,11 @@ public Tristate and(Tristate other) {
public Tristate or(Tristate other) {
return other == TRUE ? TRUE : FALSE;
}

@Override
public Tristate not() {
return TRUE;
}
},
UNDEFINED(false) {
@Override
Expand All @@ -60,6 +72,11 @@ public Tristate and(Tristate other) {
public Tristate or(Tristate other) {
return other;
}

@Override
public Tristate not() {
return UNDEFINED;
}
};

private final boolean val;
Expand All @@ -69,7 +86,7 @@ public Tristate or(Tristate other) {
}

/**
* Return the appropriate tristate for a given boolean value.
* Returns the appropriate tristate for a given boolean value.
*
* @param val The boolean value
* @return The appropriate tristate
Expand All @@ -78,6 +95,21 @@ public static Tristate fromBoolean(boolean val) {
return val ? TRUE : FALSE;
}

/**
* Returns the appropriate tristate for a given nullable boolean value,
* where null equates to {@link Tristate#UNDEFINED}.
*
* @param val The nullable boolean value
* @return The appropriate tristate
*/
public static Tristate fromNullable(@Nullable Boolean val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be named fromNullableBoolean.

If we have this, should we also have an asNullableBoolean analogue?

Copy link
Member Author

@ItsDoot ItsDoot Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

See, I was thinking about doing asNullableBoolean, but what should it return? Returning an actual nullable boolean would make sense, but Sponge prefers Optionals... which to me would feel a little weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of those rare cases I'd have a nullable return type as a one-to-one mapping of Boolean<->Tristate - if you can get a Tristate from a Boolean, it stands to reason that you should be able to go the other way just as simply.

I'll let @Zidane call this one, but I think it should just be a @Nullable Boolean return type. There is an extra clue in the name of the method here, if you're not expecting the possibility of it being null if you grab the Boolean then that's really your lookout.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer, not to have a asNullableBoolean, because this class is the replacement for the nullable/unset boolean.

As for the fromX methods, I would rather name them both from (or fromBoolean) with just different parameters. Let the compiler decide which one it should use.

Copy link
Contributor

Choose a reason for hiding this comment

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

because this class is the replacement for the nullable/unset boolean.

That doesn't change my argument at all. I'm saying the mapping should work both ways in case you then interact with some system that isn't Tristate aware - plugins can (and often do) work with third party libraries.

Copy link
Member Author

@ItsDoot ItsDoot Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Changed fromNullable to fromNullableBoolean, and added asNullableBoolean.

As for the fromX methods, I would rather name them both from (or fromBoolean) with just different parameters. Let the compiler decide which one it should use.

@ST-DDT I'd rather users explicitly opt-in to using the nullable boolean conversions. Then it's clear when someone else is reading your code, for example.

if (val == null) {
return UNDEFINED;
} else {
return val ? TRUE : FALSE;
}
}

/**
* ANDs this tristate with another tristate.
*
Expand All @@ -94,6 +126,13 @@ public static Tristate fromBoolean(boolean val) {
*/
public abstract Tristate or(Tristate other);

/**
* Negates this tristate.
*
* @return The result
*/
public abstract Tristate not();

/**
* Returns the boolean representation of this tristate.
*
Expand Down