Re: [guava] Use of Optional in cases where null has a single meaning

[Willi Schönborn]

I don't have an answer for you, but I am really curious how you are using your two-dimensional key-space. Isn't a map usually enough?

On 9 Apr 2013 04:44, <x...@barrier9.net> wrote:

I think this question is better suited for here than StackOverflow because it's more about opinion and best practice than a black-and-white facts, but apologies if it's in the wrong place.

I have a settings class that is backed by two Table<String,String,Object> instances for 'settings' and 'defaults'. When retrieving a setting, the value at the given String keys in 'settings' is returned. If it's unset, the value at the same place in 'defaults' is returned. If that too is absent, an exception is thrown.

In this arrangement, a null value has a single defined meaning, that a key has been set but the value is deliberately empty. Null never means that the key is not set. However, because HashBasedTable doesn't support null values, I'm looking at implementing Optional<Object> for the value type instead.

I have two options, and I'm wondering which one would be the better practice:

1. Have the settings class get and set Optional<Object> instances directly without modification. This would require classes making use of the settings class to wrap and unwrap values themselves first.

2. Have the settings class get and set Objects, and handle the wrapping into Optional<Object> internally. ie. null would be converted to Optional.absent() and vice versa.

If null was ambiguous in this case as it is in Map (ie. is null intentional or does it mean the key doesn't exist?) then the first choice would be obvious, but because null has a single fixed meaning in this case, I'm not sure if exposing the Optional to the rest of the application would be better practice, or having the settings class handle it internally.

Thanks,

Mike

--
--
guava-...@googlegroups.com
Project site: http://guava-libraries.googlecode.com
This group: http://groups.google.com/group/guava-discuss
 
This list is for general discussion.
To report an issue: http://code.google.com/p/guava-libraries/issues/entry
To get help: http://stackoverflow.com/questions/ask (use the tag "guava")
 
---
You received this message because you are subscribed to the Google Groups "guava-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to guava-discus...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Torbjorn Gannholm]

My opinion: go with 2 since you already have an exception to define the "not found" case.

[Michael Moore]

Willi,

Google Groups seems to be complaining about server connections when I try to post there so hopefully reply by email works.

Writing out how it works has basically convinced me it's a Bad Idea that needs a rewrite, but here's what it is right now anyway: The Settings class is implemented a little oddly. Each setting is associated with a single level group string that acts as an instance discriminator when mapping to a database source. The settings and defaults tables are both static and are used as <group, key, value>. There's also a static Map<Settings,String> that maps instances of Settings to a group string. Doing new Settings("blah") registers that instance of Settings to the "blah" group name, and then subsequent calls to Map-style methods like get(key) or put(key, value) on the Settings instance pull the group string from the groups map automatically (basically just a String group = groups.get(this)) and then pass it on to the static 'settings' table with put(group, key, value).

I ended up with static tables to save me having to use a manager class to look after the 'Settings instance'-to-'group string' mapping. In retrospect, implementing it this way seems almost anti-OO. The perils of trying to design solutions for problems at ridiculous hours of the morning, I suppose. I'm going to rethink the whole thing.

That said, the original question is I think still a valid one, and I appreciate the answer(s).

[Jens]

Seems a bit odd to me that "default" values can be missing. That somehow disqualifies them from being called "default" values.

As you are throwing an exception if settings and defaults are null for a given key, the caller of your class never deals with nulls. The caller gets a value or has to deal with the exception. In that case 1.) doesn't buy you anything. It just makes your public API more complex.

Also I am not sure if I would use 2.) as no one has access to your class internals. I tend to use Optional for public methods that can be accessed by anyone and not inside "data structures" that rarely change.

I doesn't really see anything wrong with two null checks inside your class implementation, especially if null only has a single meaning in your code. 

-- J.

[Michael Moore]

Hi Jens. This may be moot as I'm planning to redesign the class
tomorrow, but to address what you raised: The defaults table is being
used as a key validator of sorts as well. Possible settings are added
to defaults during construction, after which they can't be changed. If
a given key doesn't exist in defaults, that key is not a valid setting
and can't be set in the settings table either, and throws an exception
if someone attempts to use it. That doesn't mean the value for that
setting can't be null, however, which is why simply not including the
key in the defaults table for null values wasn't an option.

I think that null being considered a valid value isn't unreasonable in
this particular case. An example might be storing a setting to define
a boundary shape. To represent the case where there is no desired
boundary, the setting would need to be able to store that in some way,
either as a null value, Optional.absent() or a NullShape object. I
find the Null Object pattern distasteful so I'd prefer to avoid that
where possible.

[Jens]

Ah I see, so you have an immutable set of keys along with their (nullable) default values and some sort of an InvalidKeyException. I previously thought your exception means that a setting hasn't been set and does not have a default value.

In that case I would use Optional as return type for Settings.get(key). I would not use Optional for Settings.set(key, value). Instead it should check that both parameters are not null and in addition I would add Settings.clear(key). That way you are forced to clear a setting instead of setting it to null. Seems more natural to me. It also hides how "empty/cleared" settings are handled internally.

In general I would never use Optional as a method parameter as its somewhat annoying for the calling code. If you can't introduce Settings.clear(key) then add a @Nullable annotation to Settings.set() and document that null is a valid value.

Also I would use an enum as settings key. That way, your settings class do not need to throw an InvalidKeyException. But that probably means a bit more refactoring.

-- J.