Hi Peter,
Now that you have various findByXXX methods return Optional<Level>, you
could make methods that use them more Optional-API-powered. For example,
findLevel could be written as a single expression:
Right. But do you mind if I leave that for another day?
I'm not too keen on multi-line code statements embedded
in lambdas - but I feel like we did enough restructuring
for this fix now ;-)
From your other mail:
Oh, yes. I missed Chris' comment. Perhaps just add a comment about that for
posterity?
I added the following comment to stress out that
a reachability fence is not needed (at the two places
where it occurred):
479 // add new Level.
480 Level levelObject = new Level(name, x);
481 // There's no need to use a reachability fence here because
482 // KnownLevel keeps a strong reference on the level when
483 // level.getClass() == Level.class.
484 return KnownLevel.findByValue(x, KnownLevel::referent).get();
For the record here are the changes - including Mandy's suggestions.
If there's no objection I will push them after testing has passed,
since Mandy said she didn't require a new webrev.
http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.03
best regards,
-- daniel
On 16/08/16 14:54, Peter Levart wrote:
Hi Daniel,
Now that you have various findByXXX methods return Optional<Level>, you
could make methods that use them more Optional-API-powered. For example,
findLevel could be written as a single expression:
static Level findLevel(String name) {
return KnownLevel
.findByName(Objects.requireNonNull(name), KnownLevel::mirrored)
.or(() -> {
// Now, check if the given name is an integer. If so,
// first look for a Level with the given value and then
// if necessary create one.
try {
int x = Integer.parseInt(name);
return KnownLevel
.findByValue(x, KnownLevel::mirrored)
.or(() -> {
// add new Level
new Level(name, x);
return KnownLevel.findByValue(x,
KnownLevel::mirrored);
});
} catch (NumberFormatException ex) {
// Not an integer.
// Drop through.
return Optional.empty();
}
})
.or(() -> KnownLevel.findByLocalizedLevelName(name,
KnownLevel::mirrored))
.orElse(null);
}
Same with parse:
public static synchronized Level parse(String name) throws
IllegalArgumentException {
return KnownLevel
// Look for a known Level with the given non-localized name.
.findByName(Objects.requireNonNull(name), KnownLevel::referent)
.or(() -> {
// Now, check if the given name is an integer. If so,
// first look for a Level with the given value and then
// if necessary create one.
try {
int x = Integer.parseInt(name);
return KnownLevel
.findByValue(x, KnownLevel::referent)
.or(() -> {
// add new Level.
new Level(name, x);
return KnownLevel.findByValue(x,
KnownLevel::referent);
});
} catch (NumberFormatException ex) {
// Not an integer.
// Drop through.
return Optional.empty();
}
})
// Finally, look for a known level with the given localized
name,
// in the current default locale.
// This is relatively expensive, but not excessively so.
.or(() -> KnownLevel.findByLocalizedLevelName(name,
KnownLevel::referent))
// OK, we've tried everything and failed
.orElseThrow(() -> new IllegalArgumentException("Bad level
\"" + name + "\""));
}
Regards, Peter
On 08/16/2016 12:42 PM, Daniel Fuchs wrote:
Hi Mandy,
I added an additional selector parameter to the find methods.
This made it possible to return Optional<Level> instead of
KnownLevel - and it does simply the parse() method.
http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02
best regards,
-- daniel
On 11/08/16 20:12, Mandy Chung wrote:
On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fu...@oracle.com>
wrote:
On 10/08/16 17:21, Mandy Chung wrote:
On Jul 29, 2016, at 4:54 AM, Daniel Fuchs
<daniel.fu...@oracle.com> wrote:
http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/
This looks pretty good.
Since KnownLevel is now a Reference, I suggest to change
KnownLevel::findByName, findByValue and findByLocalizedLevelName to
return Optional<Level> instead such that the parse method
implementaiton could be simplified.
We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.
So either findByName(String name, boolean mirror) or two methods:
findLevelByName and findMirroredLevelByName??
Or seriously consider to remove KnownLevel class by introducing a new
Level subclass with final Level.getName, Level.getLocalizedName,
Level.getResourceBundleName methods??
Mandy
