Replace custom getOrDefault() with the corresponding Java method
#10370
Conversation
This commit replaces the `ResTable_config#getOrDefault()` methods, with `Map#getOrDefault()`.
There was a problem hiding this comment.
Code Review
This pull request aims to simplify the codebase by replacing a custom getOrDefault method with the standard Map#getOrDefault from Java 8. This is a good initiative for maintainability. However, there's a subtle but potentially important difference in behavior regarding null values stored in the map, which needs clarification.
Summary of Findings
- Potential Behavioral Change with Null Values: The switch from the custom
getOrDefaulttoMap#getOrDefaultmight change behavior if the maps involved can storenullvalues for keys. The custom method was designed to return the default value even if a key was explicitly mapped tonull, whereasMap#getOrDefaultreturnsnullin such cases. This needs clarification to ensure no unintended regressions orNullPointerExceptionsare introduced.
Merge Readiness
The intent to use standard library methods is commendable. However, due to the potential behavioral change highlighted concerning null values in maps, I recommend that this pull request not be merged until this point is clarified and, if necessary, addressed. If the maps are guaranteed not to hold null values, then the change is safe. Otherwise, further adjustments might be needed. As a reviewer, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers after addressing any concerns.
| @@ -1222,32 +1222,32 @@ public final Map<Type, String> toStringParts() { | |||
| result.put(Type.LOCALE_VARIANT_STRING, localeVariantString()); | |||
| result.put( | |||
| Type.SCREEN_LAYOUT_DIRECTION, | |||
| getOrDefault(SCREENLAYOUT_LAYOUTDIR_VALUES, screenLayoutDirection(), "")); | |||
| SCREENLAYOUT_LAYOUTDIR_VALUES.getOrDefault(screenLayoutDirection(), "")); | |||
There was a problem hiding this comment.
The custom getOrDefault method that was removed had a specific documented behavior regarding keys mapped to null values. The comment stated: // Null is not returned, even if the map contains a key whose value is null. This is intended. This implies that if map.get(key) resulted in null (either because the key was absent OR because the key was present and explicitly mapped to null), the defaultValue would be returned.
Standard Map.getOrDefault(key, defaultValue) behaves differently in one of these scenarios: if key is present in the map and its value is null, Map.getOrDefault will return null, not the defaultValue.
Could you please clarify if the various XXX_VALUES maps (e.g., SCREENLAYOUT_LAYOUTDIR_VALUES, DENSITY_DPI_VALUES, etc.) are guaranteed not to contain explicit null values for any of their keys?
- If these maps never store
nullas a value for a key (i.e., if a key is present, its value is non-null), then this change is perfectly fine and a good simplification. The original comment might have been overly defensive. - If these maps can store
nullas a value for a key, this change introduces a behavioral difference. For example:SCREENLAYOUT_LAYOUTDIR_VALUES.getOrDefault(screenLayoutDirection(), ""): IfscreenLayoutDirection()is a key mapped tonull, the old method returned"", butMap.getOrDefaultwill returnnull.DENSITY_DPI_VALUES.getOrDefault(density, density + "dpi"): Ifdensityis a key mapped tonull, the old method returneddensity + "dpi", butMap.getOrDefaultwill returnnull.
This difference could lead to null being propagated where a defaultValue was previously expected, potentially causing NullPointerExceptions or other unexpected behavior downstream if the consuming code isn't prepared for null.
If the latter case (maps can contain null values) is possible and the original behavior of returning defaultValue even for explicit null values is desired, then Map.getOrDefault might not be a direct semantic replacement, and the logic might need adjustment to preserve the original intent.
This commit replaces the
ResTable_config#getOrDefault()methods, withMap#getOrDefault().