feat : java-sdk main implementation #53
Conversation
Yg-Hong
commented
Feb 28, 2025
Yg-Hong
commented
- Add LightSwitchClient (Core class responsible for feature flagging function)
- Add LightSwitchClientConfig
- Add FeatureFlag (model class)
- Add LightSwitchUser
- Add test code
| throw new LightSwitchCastException(key, value, expectedType); | ||
| } | ||
| return expectedType.cast(value); | ||
| } |
There was a problem hiding this comment.
How about move all private methods to the bottom?
| if (flag == null) return defaultValue; | ||
|
|
||
| Object value = flag.evaluate(user); | ||
| return castValue(key, value, Boolean.class); |
There was a problem hiding this comment.
Since 58~62 lines are duplicated to other methods too, it would be better to extract private method for these.
|
|
||
| public FeatureFlag(String key, Object defaultValue, Map<Map<String, String>, Object> filters) { | ||
| this.key = key; | ||
| this.defaultValue = defaultValue; |
There was a problem hiding this comment.
Doesn't defaultValue also require Null check?
|
|
||
| boolean getBooleanFlag(String key, boolean defaultValue, LightSwitchUser user); | ||
|
|
||
| int getIntFlag(String key, int defaultValue, LightSwitchUser user); |
There was a problem hiding this comment.
I'm curious about why the flag restricts the number type to Int. It seems like there could be many cases where storing a Long would be necessary.
| private final String serverUrl; | ||
| private final String sdkKey; | ||
| private final int reconnectDelay; | ||
| private final int connectionTimeout; |
There was a problem hiding this comment.
I think it would be better if the client could customize the read timeout as well.
| if (serverUrl == null) { | ||
| throw new LightSwitchConfigException("Server URL cannot be null"); | ||
| } |
There was a problem hiding this comment.
How about using StringUtils to check if the server URL is blank as well?
| for (Map.Entry<Map<String, String>, Object> entry : filters.entrySet()) { | ||
| Map<String, String> conditions = entry.getKey(); | ||
| boolean match = conditions.entrySet().stream() | ||
| .allMatch(cond -> user.getAttributes().getOrDefault(cond.getKey(), "").equals(cond.getValue())); | ||
|
|
||
| if (match) { | ||
| return entry.getValue(); | ||
| } | ||
| } | ||
| return defaultValue; |
There was a problem hiding this comment.
If we simplify the logic using Streams, it might be more readable than using a for loop and manually iterating.
return filters.entrySet().stream()
.filter(entry -> entry.getKey().entrySet().stream()
.allMatch(cond -> user.getAttributes().getOrDefault(cond.getKey(), "").equals(cond.getValue())))
.map(Map.Entry::getValue)
.findFirst()
.orElse(defaultValue);
|
|
||
| private final String key; | ||
| private final Object defaultValue; | ||
| private final Map<Map<String, String>, Object> filters; // filtering rules |
There was a problem hiding this comment.
Now that the filters have been added to flag, it seems like similar modifications should be needed in the backend entity and API spec as well. It would be good to review and align everything before moving forward.
| public class LightSwitchUser { | ||
|
|
||
| private final String userId; | ||
| private final Map<String, String> attributes; |
There was a problem hiding this comment.
It looks like LightSwitchUser serves a different purpose than the backend's User, where is this data stored?
