feat: refactor to allow for parsing of any single leaf or array of leafs #235
Conversation
Pull Request Test Coverage Report for Build 758
💛 - Coveralls |
core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java
Show resolved
Hide resolved
| Logger.getAnonymousLogger().log(Level.ALL, "problem parsing audience conditions", e); | ||
| } | ||
| try { | ||
| conditions = ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, jsonCondition); |
There was a problem hiding this comment.
@nchilada this is the only other one that will expect any type. audience.conditions as we mentioned in the ticket and offline will remain legacy.
There was a problem hiding this comment.
Looks good, as far as I can tell. Thanks again @thomaszurkan-optimizely!
There was a problem hiding this comment.
Guess I was too late to the punch and this was merged. Here's my original summary:
Left a handful of comments. Most of them are about code style and indent. I realize that most of that can be attributed to existing code, but since we are shuffling things around, it would be a great time to clean it up. IDE should make this very easy to do on a line-by-line basis.
My main feedback is to reconsider the use of method overloading with the the Object type. For me, this carries a strong code smell because there's almost always a better approach.
| conditions = ConditionUtils.parseConditions(UserAttribute.class, rawObjectList); | ||
| } | ||
| else if (conditionsElement.isJsonObject()) { | ||
| Object object = gson.fromJson(conditionsElement,Object.class); |
| List<Object> rawObjectList = gson.fromJson(conditionsElement, List.class); | ||
| conditions = ConditionUtils.parseConditions(UserAttribute.class, rawObjectList); | ||
| } | ||
| else if (conditionsElement.isJsonObject()) { |
There was a problem hiding this comment.
Nit: predominant convention in this repo is to have } else if (...) { on one line
| if (conditionsObject instanceof String) { // should always be true | ||
| JSONTokener tokener = new JSONTokener((String)conditionsObject); | ||
| char token = tokener.nextClean(); | ||
| if (token =='[') { |
| } | ||
|
|
||
| JSONObject conditionMap = (JSONObject)object; | ||
| return new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"), |
There was a problem hiding this comment.
Nit: space after (String)
| @@ -32,6 +32,75 @@ | |||
| import java.util.Map; | |||
|
|
|||
| public class ConditionUtils { | |||
|
|
|||
| static public <T> Condition parseConditions(Class<T> clazz, Object object) throws InvalidAudienceCondition { | |||
There was a problem hiding this comment.
IMO overloading a method that's already overloaded (twice) with Object type is not conducive to code that's easy to reason or pleasant to work with. Would be better off with a different method name.
Second. A method as complex as this needs to be clearly documented. There are lots of branches in here and right now it just looks like a kitchen sink.
| if (conditionMap.has("value")) { | ||
| value = conditionMap.get("value"); | ||
| } | ||
| return new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"), |
There was a problem hiding this comment.
indent of this line looks off.
| throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName())); | ||
|
|
||
| } | ||
| org.json.JSONObject conditionMap = (org.json.JSONObject)object; |
There was a problem hiding this comment.
nit: space after (org.json.JSONObject)
Summary
The "why", or other context.
For consistency across SDKs
Test plan
No additional tests were written.
Issues